2680 m2m fields refactor #2716

BCerki · 2025-01-21T19:53:07Z

card: https://github.com/orgs/bcgov/projects/123/views/16?pane=issue&itemId=93581679&issue=bcgov%7Ccas-registration%7C2680

Notes:

I've made the operator field on contacts optional to support reg1
I also deleted user_documents and operator_documents in addition to what's in the card's AC
Where I've refactored from operation_baker() (etc) to baker recipes, it's because operation_contact() now creates an operator, and that can make it hard to keep track of counts of things (e.g. the facility tests were failing because if there's an operation_baker and an operation_contact, we end up with two operations, two addresses, etc.)
In the v2 mock data, I've checked that all operators have a contact, and operation contacts have to belong to the operation's operator
Sonar failures are repetition, which is ok because it'll get fixed when we remove the reg1 stuff
I had to make a couple changes to v1, but luckily we weren't using the fields that I changed/deleted, so the e2e tests and the app still seem to be happy
I manually clicked around in both reg1 and reg2 to try and break anything operation/contact/document related

BCerki · 2025-01-24T22:45:27Z

bc_obps/registration/models/operator.py

-    documents = models.ManyToManyField(
-        Document,
-        blank=True,
-        related_name="operators",
-    )
-    contacts = models.ManyToManyField(
-        Contact,
-        blank=True,
-        related_name="operators",
-    )


I don't think we're using either of these (@Sepehr-Sobhani , would you mind checking metabase? I don't have access to prod data)

BCerki · 2025-01-24T22:45:39Z

bc_obps/registration/models/user.py

@@ -13,11 +13,6 @@ class User(UserAndContactCommonInfo):
        max_length=1000,
        db_comment="The name of the business the user is associated with as per their Business BCeID account",
    )
-    documents = models.ManyToManyField(


@Sepehr-Sobhani also this one

andrea-williams · 2025-01-27T19:40:45Z

bc_obps/registration/models/document.py

@@ -32,3 +40,5 @@ def delete(self, *args, **kwargs):
            default_storage.delete(self.file.name)

        super().delete(*args, **kwargs)
+
+    #


tedious nit: seems unnecessary?

andrea-williams · 2025-01-27T19:57:35Z

bc_obps/service/user_operator_service.py

@@ -277,6 +277,7 @@ def update_status(
        if user_operator.status == UserOperator.Statuses.DECLINED:
            # Set role to pending for now but we may want to add a new role for declined
            user_operator.role = UserOperator.Roles.PENDING
+            # brianna you're going to have to go deal with thiss


do you plan to deal with "thiss" in this PR or a follow up one?

I didn't have to deal with it in the end, actually, nothing in reg1 broke because we learned you can use set/add for both m2m and fk

andrea-williams · 2025-01-27T19:58:03Z

bciers/apps/administration/app/components/contacts/ContactsDataGrid.tsx

    return row.id + operator;
  }
-
+  console.log("initialdata", initialData);


andrea-williams · 2025-01-27T21:28:16Z

...ion/tests/endpoints/v2/_user_operators/_user_operator_id/test_user_operator_update_status.py

@@ -149,6 +149,7 @@ def test_cas_director_approves_admin_access_request_with_new_operator(self, mock
        )

    def test_cas_analyst_declines_access_request(self, mocker):
+        # brianna this will be fixed after rebase


sorry, leftover

andrea-williams · 2025-01-27T21:44:51Z

bc_obps/service/contact_service_v2.py

+    def create_contact(cls, user_guid: UUID, payload: Union[ContactIn, OperationRepresentativeIn]) -> Contact:
+        contact_data: dict = payload.dict(include={*ContactIn.Meta.fields})
+        # `business_role` is a mandatory field in the DB but we don't collect it from the user
+        # so we set it to a default value here and we can change it later if needed


I think we can probably delete the business_role field on Contact, I don't think we use it anywhere? (but this is tech debt we can leave for later)

There are a couple places where we filter by business_role Operation Representative

andrea-williams · 2025-01-27T21:58:58Z

bc_obps/service/contact_service_v2.py

+            existing_contact_address = contact.address
+            if existing_contact_address:
+                contact.address = None
+                contact.save(update_fields=['address'])
+                # contact has an address and the payload has no address data, remove the address
+                existing_contact_address.delete()


I find this part confusing and overly complex. Couldn't it be simplified to:

if contact.address: contact.address.delete() contact.address = None # not even sure this line is needed contact.save(update_fields=['address'])

side note - I'm having major deja vu reading this code. Was this copied over from v1 of the service? If so there might be an argument for keeping this code as-is until we throw away all our v1 code.

Yes, it's copied from v1. I'll leave it as-is for now

andrea-williams · 2025-01-27T22:11:35Z

bc_obps/service/data_access_service/document_service_v2.py

+
+class DocumentDataAccessServiceV2:
+    @classmethod
+    def get_operation_document_by_type(cls, operation_id: UUID, document_type: str) -> Document | None:


I got lost trying to follow the paths of where this function gets called, so apologies if this question is wrong, but do we need to add a check here based on user_guid to confirm that the user has permission to access data for the operation in question? (I couldn't tell if this gets checked somewhere else)

I made a get_if_authorized function to this, that's how we've generally done it in other services. Thanks for catching this!

andrea-williams · 2025-01-27T22:13:46Z

bc_obps/service/tests/test_document_service_v2.py

+        assert created is False
+
+    @staticmethod
+    def test_update_operation_document():


could I request another unit test here to confirm that an industry_user is NOT allowed to update/view a document associated with an operation outside of their operator? (this relates to my previous comment above)

andrea-williams

Wonderful! Thanks for adding that extra unit test in <3

BCerki force-pushed the 2680-m2m-fields-refactor branch 5 times, most recently from f331364 to 2e0c8af Compare January 24, 2025 21:54

BCerki commented Jan 24, 2025

View reviewed changes

BCerki marked this pull request as ready for review January 24, 2025 22:49

andrea-williams reviewed Jan 27, 2025

View reviewed changes

BCerki force-pushed the 2680-m2m-fields-refactor branch 4 times, most recently from adedb9b to 5113692 Compare January 28, 2025 22:07

andrea-williams approved these changes Jan 30, 2025

View reviewed changes

BCerki force-pushed the 2680-m2m-fields-refactor branch 3 times, most recently from a4bbe6e to 9dfe44c Compare January 31, 2025 16:02

BCerki added 8 commits January 31, 2025 08:14

chore: remove documents and contacts m2m relationships

272d295

chore: update v2 fixtures

3e74329

chore: update schemas, services, endpoints

bbf14ae

test: pytests

470e678

chore: update v1 and test fixtures

b32ceaa

chore: update contacts grid

c30d29c

chore: apply suggestions from code review

1f3e7ff

chore: post-rebase updates

82a20d2

BCerki force-pushed the 2680-m2m-fields-refactor branch from 9dfe44c to 82a20d2 Compare January 31, 2025 16:14

BCerki merged commit 4189377 into develop Jan 31, 2025
41 of 42 checks passed

BCerki deleted the 2680-m2m-fields-refactor branch January 31, 2025 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2680 m2m fields refactor #2716

2680 m2m fields refactor #2716

BCerki commented Jan 21, 2025 •

edited

Loading

BCerki Jan 24, 2025

Sepehr-Sobhani Jan 24, 2025

BCerki Jan 24, 2025

Sepehr-Sobhani Jan 24, 2025

andrea-williams Jan 27, 2025

andrea-williams Jan 27, 2025

BCerki Jan 28, 2025

andrea-williams Jan 27, 2025

andrea-williams Jan 27, 2025

BCerki Jan 28, 2025

andrea-williams Jan 27, 2025

BCerki Jan 28, 2025

andrea-williams Jan 27, 2025

BCerki Jan 28, 2025

andrea-williams Jan 27, 2025

BCerki Jan 28, 2025

andrea-williams Jan 27, 2025

andrea-williams left a comment

2680 m2m fields refactor #2716

2680 m2m fields refactor #2716

Conversation

BCerki commented Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-williams left a comment

Choose a reason for hiding this comment

BCerki commented Jan 21, 2025 •

edited

Loading