Merge pull request #499 from awf-dbca/todo

Addressing TODOs

boranga/components/occurrence/api.py

@@ -2155,7 +2155,6 @@ def process_shapefile_document(self, request, *args, **kwargs):
     def validate_map_files(self, request, *args, **kwargs):
         instance = self.get_object()
         validate_map_files(request, instance, "occurrence_report")
-        # TODO: determine what this actually changes
         if instance.processing_status == OccurrenceReport.PROCESSING_STATUS_UNLOCKED:
             self.unlocked_back_to_assessor()
             instance.save(no_revision=True)
@@ -3053,10 +3052,7 @@ def create(self, request, *args, **kwargs):
 
 class GetOCCProfileDict(views.APIView):
     def get(self, request, format=None):
-        group_type = request.GET.get("group_type", "")
-        logger.debug(
-            "group_type: %s" % group_type
-        )  # TODO: Unused variable here. Use or remove.
+
         wild_status_list = list(WildStatus.objects.all().values("id", "name"))
         occurrence_source_list = list(Occurrence.OCCURRENCE_SOURCE_CHOICES)
 
@@ -3358,7 +3354,7 @@ def occurrence_name_lookup(self, request, *args, **kwargs):
         detail=False,
     )
     def combine_occurrence_name_lookup(self, request, *args, **kwargs):
-        if is_internal(self.request):  # TODO group auth
+        if is_internal(self.request):  
             main_occurrence_id = request.GET.get("occurrence_id", None)
 
             if main_occurrence_id:
@@ -4673,7 +4669,7 @@ def occurrence_save(self, request, *args, **kwargs):
             for i in site_geometry_data["features"]:
                 try:
                     update_site = occ_sites.get(
-                        site_number=i["properties"]["site_number"]
+                        id=i["id"]
                     )
                     point_data = "POINT({} {})".format(
                         i["geometry"]["coordinates"][0], i["geometry"]["coordinates"][1]
@@ -4693,7 +4689,7 @@ def occurrence_save(self, request, *args, **kwargs):
 
                     update_site.geometry = geom_4326
                     update_site.original_geometry_ewkb = geom_original
-                    update_site.save()  # TODO add version_user when history implemented
+                    update_site.save(version_user=request.user) 
                 except Exception as e:
                     print(e)
 
@@ -4796,7 +4792,7 @@ def update_location_details(self, request, *args, **kwargs):
             for i in site_geometry_data["features"]:
                 try:
                     update_site = occ_sites.get(
-                        site_number=i["properties"]["site_number"]
+                        id=i["id"]
                     )
                     point_data = "POINT({} {})".format(
                         i["geometry"]["coordinates"][0], i["geometry"]["coordinates"][1]
@@ -4816,7 +4812,7 @@ def update_location_details(self, request, *args, **kwargs):
 
                     update_site.geometry = geom_4326
                     update_site.original_geometry_ewkb = geom_original
-                    update_site.save()  # TODO add version_user when history implemented
+                    update_site.save(version_user=request.user)  
                 except Exception as e:
                     print(e)
 

boranga/components/occurrence/models.py

@@ -567,8 +567,6 @@ def can_change_lock(self, request):
             OccurrenceReport.PROCESSING_STATUS_UNLOCKED,
             OccurrenceReport.PROCESSING_STATUS_APPROVED,
         ]:
-            # TODO: current requirment task allows assessors to unlock, is this too permissive?
-            # Good question
             return (
                 is_occurrence_assessor(request)
                 or is_occurrence_approver(request)
@@ -1711,6 +1709,7 @@ def __str__(self):
         return str(self.name)
 
 
+# NOTE: this and OCCLocation have a number of unused fields that should be removed
 class OCRLocation(models.Model):
     """
     Location data  for occurrence report
@@ -1728,7 +1727,7 @@ class OCRLocation(models.Model):
     boundary_description = models.TextField(null=True, blank=True)
     new_occurrence = models.BooleanField(
         null=True, blank=True
-    )  # TODO what is this for? is it needed?
+    )  
     boundary = models.IntegerField(null=True, blank=True, default=0)
     mapped_boundary = models.BooleanField(null=True, blank=True)
     buffer_radius = models.IntegerField(null=True, blank=True, default=0)
@@ -4073,8 +4072,6 @@ class OccurrenceGeometry(GeometryBase, DrawnByGeometry, IntersectsGeometry):
         related_name="occ_geometry",
     )
     locked = models.BooleanField(default=False)
-    # TODO: possibly remove buffer radius from location models
-    # when we go with the radius being a property of the geometry
     buffer_radius = models.FloatField(null=True, blank=True, default=0)
 
     color = ColorField(default="#3333FF")  # Light blue
@@ -4795,15 +4792,15 @@ class OccurrenceTenure(RevisionedMixin):
 
     def save(self, *args, **kwargs):
 
-        # force_insert = kwargs.pop("force_insert", False)
-        # if force_insert:
-        #    super().save(no_revision=True, force_insert=force_insert) #TODO enable when we have history
-        #    self.save(*args, **kwargs)
-        # else:
-        override_datetime_updated = kwargs.pop("override_datetime_updated", False)
-        if not override_datetime_updated:
-            self.datetime_updated = datetime.now()
-        super().save(*args, **kwargs)
+        force_insert = kwargs.pop("force_insert", False)
+        if force_insert:
+           super().save(no_revision=True, force_insert=force_insert)
+           self.save(*args, **kwargs)
+        else:
+            override_datetime_updated = kwargs.pop("override_datetime_updated", False)
+            if not override_datetime_updated:
+                self.datetime_updated = datetime.now()
+            super().save(*args, **kwargs)
 
     class Meta:
         app_label = "boranga"

boranga/components/occurrence/permissions.py

@@ -41,7 +41,7 @@ def has_object_permission(self, request, view, obj):
                 .exists()
             ):
                 return True
-            # TODO edge case, consider not using POST for process_shapefile_document if possible
+            # NOTE replace/remove when process_shapefile_document is reworked
             elif (
                 hasattr(view, "action") and view.action == "process_shapefile_document"
             ):

boranga/components/occurrence/serializers.py

@@ -3242,7 +3242,7 @@ def get_details_url(self, obj):
         request = self.context["request"]
 
         if request.user.is_authenticated:
-            # TODO: Don't have these url names yet
+            # NOTE: review if needed
             if is_internal(request):
                 return f"{obj.id}"
                 # return reverse(
@@ -3251,10 +3251,6 @@ def get_details_url(self, obj):
                 # )
             else:
                 return None
-                # return reverse(
-                #     "external-occurrence-detail",
-                #     kwargs={"occurrence_pk": obj.id},
-                # )
 
         return None
 

boranga/components/species_and_communities/api.py

@@ -1444,19 +1444,6 @@ def species_split_save(self, request, *args, **kwargs):
             if serializer.is_valid():
                 serializer.save()
 
-        # TODO @Ash - move this to dedicated save and replace with setting to private
-        if request_data.get("publishing_status"):
-            publishing_status_instance, created = (
-                SpeciesPublishingStatus.objects.get_or_create(species=instance)
-            )
-            serializer = SaveSpeciesPublishingStatusSerializer(
-                publishing_status_instance,
-                data=request_data.get("publishing_status"),
-            )
-            serializer.is_valid(raise_exception=True)
-            if serializer.is_valid():
-                serializer.save()
-
         serializer = SaveSpeciesSerializer(instance, data=request_data, partial=True)
         serializer.is_valid(raise_exception=True)
         if serializer.is_valid():

boranga/components/species_and_communities/models.py

@@ -1956,29 +1956,7 @@ def add_documents(self, request, *args, **kwargs):
             self.save(no_revision=True)  # no need to have multiple revisions
         # end save documents
         self.save(*args, **kwargs)
-
-    # TODO: review - may not need this (?)
-    @property
-    def reversion_ids(self):
-        current_revision_id = Version.objects.get_for_object(self).first().revision_id
-        versions = (
-            Version.objects.get_for_object(self)
-            .select_related("revision__user")
-            .filter(
-                Q(revision__comment__icontains="status")
-                | Q(revision_id=current_revision_id)
-            )
-        )
-        version_ids = [[i.id, i.revision.date_created] for i in versions]
-        return [
-            dict(
-                cur_version_id=version_ids[0][0],
-                prev_version_id=version_ids[i + 1][0],
-                created=version_ids[i][1],
-            )
-            for i in range(len(version_ids) - 1)
-        ]
-
+
 
 class CommunityDocument(Document):
     """

boranga/frontend/boranga/src/components/common/occurrence/occ_locations.vue

@@ -358,7 +358,6 @@ export default {
             return api_endpoints.occurrence + 'list_for_map/';
         },
         siteApiUrl: function () {
-            // TODO: Update to use the correct endpoint
             return '/api/occurrence_sites/list_for_map/';
         },
         ocrPropertyDisplayMap: function () {

boranga/frontend/boranga/src/components/internal/conservation_status/conservation_status.vue

@@ -552,7 +552,7 @@ export default {
                 && this.conservation_status_obj.assessor_mode.assessor_can_assess;
         },
         canAction: function () {
-            // TODO: Completely redo the permissions for actions on this page
+            // NOTE: Completely redo the permissions for actions on this page
             // It was a mess before and now it's even worse =D
             if (this.isFinalised) {
                 return this.conservation_status_obj

boranga/frontend/boranga/src/components/internal/meetings/meeting.vue

@@ -433,7 +433,7 @@ export default {
                 blank_fields.push('Please select End Date that is later than Start Date');
             }
             if (check_action == "submit") {
-                //TODO add validation for fields required before submit
+                //NOTE add validation for fields required before submit
             }
 
             return blank_fields

boranga/frontend/boranga/src/components/internal/occurrence/occurrence_combine.vue

@@ -481,7 +481,7 @@
                     vm.toggleKeyContacts();
                     vm.toggleTenures();
                 }, 200);
-                // set to 200 due to the tab fade (TODO: consider better handling of this)
+                // set to 200 due to the tab fade (NOTE: consider better handling of this)
                 // Note from @oak: Changing the keys means rebuilding all the components every time a tab is clicked.
                 // search codebase for "addEventListener('shown.bs.tab'" and try that method. Seems like it works very
                 // well with datatables.

boranga/views.py

@@ -378,8 +378,7 @@ def is_authorised_to_access_conservation_status_document(request, document_id):
             document_id, request.path, referee_allowed_paths
         )
 
-    if is_contributor(request):
-        # TODO: Would be nice if the document id was included in the upload path to simplify this query
+    if is_contributor(request):        
         contributor_allowed_paths = ["documents", "amendment_request_documents"]
         file_name = get_file_name_from_path(request.path)
         return (
@@ -426,39 +425,38 @@ def get_file_name_from_path(file_path):
 
 def is_authorised_to_access_document(request):
     # occurrence reports
-    or_document_id = get_file_path_id("occurrence_report", request.path)
-    if or_document_id:
+    document_or_id = get_file_path_id("occurrence_report", request.path)
+    if document_or_id:
         return is_authorised_to_access_occurrence_report_document(
-            request, or_document_id
+            request, document_or_id
         )
 
     # occurrence
-    o_document_id = get_file_path_id("occurrence", request.path)
-    if o_document_id:
-        return is_authorised_to_access_occurrence_document(request, o_document_id)
+    document_o_id = get_file_path_id("occurrence", request.path)
+    if document_o_id:
+        return is_authorised_to_access_occurrence_document(request, document_o_id)
 
     # conservation status
-    # TODO: This 'document id' is actually the conservation status id. Consider renaming these variables
-    cs_document_id = get_file_path_id("conservation_status", request.path)
-    if cs_document_id:
+    document_cs_id = get_file_path_id("conservation_status", request.path)
+    if document_cs_id:
         return is_authorised_to_access_conservation_status_document(
-            request, cs_document_id
+            request, document_cs_id
         )
 
     # meeting
-    m_document_id = get_file_path_id("meeting", request.path)
-    if m_document_id:
-        return is_authorised_to_access_meeting_document(request, m_document_id)
+    document_m_id = get_file_path_id("meeting", request.path)
+    if document_m_id:
+        return is_authorised_to_access_meeting_document(request, document_m_id)
 
     # species
-    s_document_id = get_file_path_id("species", request.path)
-    if s_document_id:
-        return is_authorised_to_access_species_document(request, s_document_id)
+    document_s_id = get_file_path_id("species", request.path)
+    if document_s_id:
+        return is_authorised_to_access_species_document(request, document_s_id)
 
     # community
-    c_document_id = get_file_path_id("community", request.path)
-    if c_document_id:
-        return is_authorised_to_access_community_document(request, c_document_id)
+    document_c_id = get_file_path_id("community", request.path)
+    if document_c_id:
+        return is_authorised_to_access_community_document(request, document_c_id)
 
     return False