Logic reversal for the seen/unseen mechanism

dashboard/admin.py

@@ -13,8 +13,8 @@
     Dataset,
     ObservationComment,
     Area,
-    ObservationView,
     Alert,
+    ObservationUnseen,
 )
 
 admin.site.site_header = f'{settings.GBIF_ALERT["SITE_NAME"]} administration'
@@ -36,9 +36,9 @@ class ObservationCommentCommentInline(admin.TabularInline):
     model = ObservationComment
 
 
-class ObservationViewInline(admin.TabularInline):
-    model = ObservationView
-    readonly_fields = ["user", "timestamp"]
+class ObservationUnseenInline(admin.TabularInline):
+    model = ObservationUnseen
+    readonly_fields = ["user"]
 
     # Make that inline read-only
     def has_change_permission(self, request, obj=None):
@@ -56,7 +56,7 @@ class ObservationAdmin(admin.OSMGeoAdmin):
     list_display = ("stable_id", "date", "species", "source_dataset")
     list_filter = ["data_import", "species"]
     search_fields = ["stable_id"]
-    inlines = [ObservationCommentCommentInline, ObservationViewInline]
+    inlines = [ObservationCommentCommentInline, ObservationUnseenInline]
 
 
 class SpeciesResource(resources.ModelResource):

dashboard/management/commands/import_observations.py

@@ -248,7 +248,10 @@ def batch_insert_observations(self, observations_to_insert: list[Observation]):
         inserted_observations = Observation.objects.bulk_create(observations_to_insert)
         self.log_with_time("Migrating linked entities")
         for obs in inserted_observations:
-            obs.migrate_linked_entities()
+            replaced = obs.migrate_linked_entities()
+            if not replaced:
+                # That's a new observation in the system, it should be marked as unseen for every user
+                obs.mark_as_unseen_for_all_users()
 
     def add_arguments(self, parser: CommandParser) -> None:
         parser.add_argument(

dashboard/models.py

@@ -7,6 +7,7 @@
 
 import html2text
 from django.conf import settings
+from django.contrib.auth import get_user_model
 from django.contrib.auth.models import AbstractUser, AnonymousUser
 from django.contrib.gis.db import models
 from django.contrib.gis.db.models.aggregates import Union as AggregateUnion
@@ -249,11 +250,11 @@ def filtered_from_my_params(
             qs = qs.filter(initial_data_import_id__in=initial_data_import_ids)
 
         if status_for_user and user:
-            ov = ObservationView.objects.filter(user=user)
+            ous = ObservationUnseen.objects.filter(user=user)
             if status_for_user == "seen":
-                qs = qs.filter(observationview__in=ov)
+                qs = qs.exclude(observationunseen__in=ous)
             elif status_for_user == "unseen":
-                qs = qs.exclude(observationview__in=ov)
+                qs = qs.filter(observationunseen__in=ous)
 
         return qs
 
@@ -322,14 +323,14 @@ def mark_as_seen_by(self, user: WebsiteUser) -> None:
 
         if user.is_authenticated:
             try:
-                self.observationview_set.get(user=user)
-            except ObservationView.DoesNotExist:
-                ObservationView.objects.create(observation=self, user=user)
+                self.observationunseen_set.filter(user=user).delete()
+            except ObservationUnseen.DoesNotExist:
+                pass
 
     def already_seen_by(self, user: WebsiteUser) -> bool | None:
         """Return True if the observation has already been seen by the user"""
         if user.is_authenticated:
-            return self.observationview_set.filter(user=user).exists()
+            return not self.observationunseen_set.filter(user=user).exists()
         else:
             return None
 
@@ -339,11 +340,11 @@ def mark_as_unseen_by(self, user: WebsiteUser) -> bool:
         :return: True is successful (most probable causes of failure: user has not seen this observation yet / user is
         anonymous)"""
         if user.is_authenticated:
-            try:
-                self.observationview_set.get(user=user).delete()
+            _, created = ObservationUnseen.objects.get_or_create(
+                observation=self, user=user
+            )
+            if created:
                 return True
-            except ObservationView.DoesNotExist:
-                return False
 
         return False
 
@@ -373,21 +374,33 @@ def set_or_migrate_initial_data_import(
         else:
             self.initial_data_import = replaced_observation.initial_data_import
 
-    def migrate_linked_entities(self) -> None:
+    def mark_as_unseen_for_all_users(self) -> None:
+        """Mark the observation as unseen for all users"""
+        # TODO: test this
+        for user in get_user_model().objects.all():
+            self.mark_as_unseen_by(user)
+
+    def migrate_linked_entities(self) -> bool:
         """Migrate existing entities (comments, ...) linked to a previous observation that share the stable ID
 
-        Does nothing if there's no replaced observation
+        Does nothing if there's no replaced observation.
+
+        Returns True if it migrated an existing observation, False otherwise
         """
         replaced_observation = self.replaced_observation
         if replaced_observation is not None:
             # 1. Migrating comments
             for comment in replaced_observation.observationcomment_set.all():
                 comment.observation = self
                 comment.save()
-            # 2. Migrating user views
-            for observation_view in replaced_observation.observationview_set.all():
-                observation_view.observation = self
-                observation_view.save()
+            # 2. Migrating seen/unseen status
+            for observation_unseen in replaced_observation.observationunseen_set.all():
+                observation_unseen.observation = self
+                observation_unseen.save()
+
+            return True
+        else:
+            return False
 
     @cached_property
     def replaced_observation(self) -> Self | None:
@@ -479,10 +492,10 @@ def as_dict(self, for_user: WebsiteUser) -> dict[str, Any]:
 
         if for_user.is_authenticated:
             try:
-                self.observationview_set.get(user=for_user)
-                d["seenByCurrentUser"] = True
-            except ObservationView.DoesNotExist:
+                self.observationunseen_set.get(user=for_user)
                 d["seenByCurrentUser"] = False
+            except ObservationUnseen.DoesNotExist:
+                d["seenByCurrentUser"] = True
 
         return d
 
@@ -601,6 +614,11 @@ def to_dict(self, include_geojson: bool):
 
 class ObservationView(models.Model):
     """
+    !! This model is deprecated, we now use ObservationUnseen instead !!
+    (ObservationUnseen is a reversed logic: we store unseen observations instead of seen ones)
+    This one shouldn't be used in the codebase anymore, however we do keep it for the
+    sake of older data migrations !!
+
     This models keeps a history of when a user has first seen details about an observation
 
     - If no entry for the user/observation pair: the user has never seen details about this observation

dashboard/tests/commands/test_import_observations.py

@@ -17,8 +17,8 @@
     Dataset,
     ObservationComment,
     User,
-    ObservationView,
     Alert,
+    ObservationUnseen,
 )
 
 THIS_SCRIPT_PATH = Path(__file__).parent
@@ -58,7 +58,7 @@ def predicate_builder_belgium(species_list: QuerySet[Species]):
 )
 class ImportObservationsTest(TransactionTestCase):
     def setUp(self) -> None:
-        user = User.objects.create_user(
+        self.user = User.objects.create_user(
             username="testuser",
             password="12345",
             first_name="John",
@@ -85,7 +85,7 @@ def setUp(self) -> None:
 
         self.alert_referencing_unused_dataset = Alert.objects.create(
             name="Alert referencing unused dataset",
-            user=user,
+            user=self.user,
         )
         self.alert_referencing_unused_dataset.datasets.add(
             self.dataset_without_observations, inaturalist
@@ -95,7 +95,7 @@ def setUp(self) -> None:
         # This observation will be replaced during the import process
         # because there's a row with the same occurrence_id and dataset_key in the DwC-A
         self.initial_di = DataImport.objects.create(start=timezone.now())
-        observation_to_be_replaced = Observation.objects.create(
+        self.observation_unseen_to_be_replaced = Observation.objects.create(
             gbif_id=1,
             occurrence_id="https://www.inaturalist.org/observations/33366292",
             source_dataset=inaturalist,
@@ -106,6 +106,17 @@ def setUp(self) -> None:
             location=Point(5.09513, 50.48941, srid=4326),
         )
 
+        self.observation_seen_to_be_replaced = Observation.objects.create(
+            gbif_id=3,
+            occurrence_id="https://www.inaturalist.org/observations/42577016",
+            source_dataset=inaturalist,
+            species=self.polydrusus,
+            date=datetime.date.today() - datetime.timedelta(days=1),
+            data_import=self.initial_di,
+            initial_data_import=self.initial_di,
+            location=Point(5.09513, 50.48941, srid=4326),
+        )
+
         # This one has no equivalent in the DwC-A
         observation_not_replaced = Observation.objects.create(
             gbif_id=2,
@@ -119,19 +130,19 @@ def setUp(self) -> None:
         )
 
         ObservationComment.objects.create(
-            author=user,
-            observation=observation_to_be_replaced,
+            author=self.user,
+            observation=self.observation_unseen_to_be_replaced,
             text="This is a comment to migrate",
         )
 
-        self.observation_view_to_migrate = ObservationView.objects.create(
-            user=user, observation=observation_to_be_replaced
+        self.observation_unseen_to_migrate = ObservationUnseen.objects.create(
+            user=self.user, observation=self.observation_unseen_to_be_replaced
         )
 
         # We also create this one, so we can check it gets deleted in the new import process, and that it doesn't prevent
         # the related observation to be deleted
-        self.observation_view_to_delete = ObservationView.objects.create(
-            user=user, observation=observation_not_replaced
+        self.observation_unseen_to_delete = ObservationUnseen.objects.create(
+            user=self.user, observation=observation_not_replaced
         )
 
     def test_initial_data_import_value_replaced(self):
@@ -380,25 +391,25 @@ def test_observation_comments_migrated(self) -> None:
         self.assertNotEqual(comment.observation_id, previous_observation_id)
         self.assertEqual(comment.observation.stable_id, previous_stable_id)
 
-    def test_observation_views_migrated(self) -> None:
-        ov = self.observation_view_to_migrate
-        previous_observation_id = ov.observation_id
-        previous_stable_id = ov.observation.stable_id
+    def test_observation_unseen_migrated(self) -> None:
+        ou = self.observation_unseen_to_migrate
+        previous_observation_id = ou.observation_id
+        previous_stable_id = ou.observation.stable_id
 
         with open(SAMPLE_DATA_PATH / "gbif_download.zip", "rb") as gbif_download_file:
             call_command("import_observations", source_dwca=gbif_download_file)
 
-        ov.refresh_from_db()
-        self.assertNotEqual(ov.observation_id, previous_observation_id)
-        self.assertEqual(ov.observation.stable_id, previous_stable_id)
+        ou.refresh_from_db()
+        self.assertNotEqual(ou.observation_id, previous_observation_id)
+        self.assertEqual(ou.observation.stable_id, previous_stable_id)
 
-    def test_unmigrated_ov_gets_deleted(self) -> None:
-        ov_id = self.observation_view_to_delete.id
+    def test_unmigrated_ou_gets_deleted(self) -> None:
+        ou_id = self.observation_unseen_to_delete.id
 
         with open(SAMPLE_DATA_PATH / "gbif_download.zip", "rb") as gbif_download_file:
             call_command("import_observations", source_dwca=gbif_download_file)
-        with self.assertRaises(ObservationView.DoesNotExist):
-            ObservationView.objects.get(id=ov_id)
+        with self.assertRaises(ObservationUnseen.DoesNotExist):
+            ObservationUnseen.objects.get(id=ou_id)
 
     def test_old_observations_deleted(self) -> None:
         """The old observations (replaced and not replaced) are deleted from the database after a new import"""
@@ -452,6 +463,40 @@ def test_gbif_request_not_necessary(self) -> None:
                 request_history = m.request_history
                 self.assertEqual(len(request_history), 0)
 
+    def test_seen_status(self) -> None:
+        """The seen/unseen status is correctly set after import"""
+        with open(SAMPLE_DATA_PATH / "gbif_download.zip", "rb") as gbif_download_file:
+            call_command("import_observations", source_dwca=gbif_download_file)
+
+        observations_after = Observation.objects.all()
+
+        # An existing seen observation has been replaced, it should be marked as seen
+        case1 = observations_after.get(
+            occurrence_id=self.observation_seen_to_be_replaced.occurrence_id
+        )
+        # Unseen not found => seen
+        with self.assertRaises(ObservationUnseen.DoesNotExist):
+            ObservationUnseen.objects.get(observation=case1, user=self.user)
+
+        # An existing unseen observation has been replaced, it should be marked as unseen
+        case2 = observations_after.get(
+            occurrence_id=self.observation_unseen_to_be_replaced.occurrence_id
+        )
+
+        # Unseen found => unseen
+        ObservationUnseen.objects.get(observation=case2, user=self.user)
+
+        # Several new observations have been added, they should be marked as unseen
+        newly_added_observations = observations_after.exclude(
+            occurrence_id__in=[
+                self.observation_unseen_to_be_replaced.occurrence_id,
+                self.observation_seen_to_be_replaced.occurrence_id,
+            ]
+        )
+        # We find a new ObservationUnseen for each new observation
+        for obs in newly_added_observations:
+            ObservationUnseen.objects.get(observation=obs, user=self.user)
+
     @override_settings(
         GBIF_ALERT={
             "GBIF_DOWNLOAD_CONFIG": {

dashboard/tests/models/test_alert.py

@@ -11,7 +11,7 @@
     Species,
     DataImport,
     Dataset,
-    ObservationView,
+    ObservationUnseen,
 )
 
 SEPTEMBER_13_2021 = datetime.datetime.strptime("2021-09-13", "%Y-%m-%d").date()
@@ -49,7 +49,10 @@ def setUpTestData(cls):
             location=Point(5.09513, 50.48941, srid=4326),  # Andenne
         )
 
-        # No ObservationView created in setupTestData(): at the beginning of test_* methods, the observation is unseen
+        # At the beginning of test_* methods, the observation is unseen
+        cls.obs_unseen = ObservationUnseen.objects.create(
+            observation=cls.observation, user=cls.user
+        )
 
     def test_unseen_observations_count_one(self):
         """There's one unseen observation (same data as test_has_unseen_observations_true())"""
@@ -63,7 +66,7 @@ def test_unseen_observations_count_zero_case_1(self):
         alert = Alert.objects.create(
             user=self.user, email_notifications_frequency=Alert.DAILY_EMAILS
         )
-        ObservationView.objects.create(observation=self.observation, user=self.user)
+        self.obs_unseen.delete()
         self.assertEqual(alert.unseen_observations_count, 0)
 
     def test_unseen_observations_count_zero_case_2(self):
@@ -89,7 +92,7 @@ def test_has_unseen_observations_false_case_1(self):
         alert = Alert.objects.create(
             user=self.user, email_notifications_frequency=Alert.DAILY_EMAILS
         )
-        ObservationView.objects.create(observation=self.observation, user=self.user)
+        self.obs_unseen.delete()
         self.assertFalse(alert.has_unseen_observations)
 
     def test_has_unseen_observations_false_case_2(self):
@@ -120,13 +123,13 @@ def test_email_should_be_sent_now_no_notifications(self):
         self.assertFalse(alert.email_should_be_sent_now())
 
     def test_email_should_be_sent_now_nothing_unseen(self):
-        """When nothing is unseen, it's not a god time for notifications"""
+        """When nothing is unseen, it's not a good time for notifications"""
 
         alert = Alert.objects.create(
             user=self.user, email_notifications_frequency=Alert.DAILY_EMAILS
         )
 
-        ObservationView.objects.create(observation=self.observation, user=self.user)
+        self.obs_unseen.delete()
 
         # Situation:
         # - No unseen observation