Replace operation rep #2679

BCerki · 2025-01-13T19:55:47Z

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

Notes:

I've used a url param to decide whether or not to show the alert on the contact page. As far as I could find online (e.g. https://medium.com/@awayyao/keep-page-state-between-page-navigation-in-nextjs-13-using-app-router-schema-f6eacd0300ad), the next router doesn't store any state or history. window.history doesn't store the previous url, and document.referer only gives the origin.
Updated the mock data so that contacts are more representative of how the app works (e.g., operations can only have contacts that belong to their operators; all registered operations have to have a contact)
because of ^, I noticed you can get to the operation administration page before the operation is registered if you go via the URL. I added a check in the endpoint
I wrote a couple comments in the card about where I haven't followed the card exactly

BCerki · 2025-01-14T22:38:37Z

bc_obps/service/operation_service_v2.py

@@ -198,6 +199,22 @@ def create_or_update_operation_v2(

        # set m2m relationships
        operation.activities.set(payload.activities)
+        if isinstance(payload, OperationInformationInUpdate):


This (vs if payload.operation_representatives) was to make mypy happy

BCerki · 2025-01-14T22:40:07Z

bc_obps/service/operation_service_v2.py

@@ -330,12 +347,6 @@ def update_operation(
            payload,
            operation_id,
        )
-
-        if payload.regulated_products:


This is already handled in create_or_update_operation_v2

Also, good catch, Thanks! 🙏
At some point, we might need to refactor create_or_update_operation_v2 service because it has many responsibilities.

I was thinking this too--in other places in the app we have separate create and update functions

BCerki · 2025-01-17T16:19:26Z

bciers/apps/administration/tests/components/operations/OperationInformationForm.test.tsx

@@ -162,7 +190,7 @@ const formData = {
  naics_code_id: 1,
  secondary_naics_code_id: 2,
  operation_has_multiple_operators: true,
-  activities: [1, 2, 3, 4, 5],
+  activities: [1, 2],


We only mocked two activities

Sepehr-Sobhani

This is dope! 🔥 I’ve left a few small suggestions, but nothing major.

Sepehr-Sobhani · 2025-01-22T21:36:34Z

bc_obps/service/contact_service_v2.py

+class PlacesAssigned(Schema):
+    role_name: str
+    operation_name: str
+    operation_id: UUID
+
+
+class ContactWithPlacesAssigned(ContactOut):
+    places_assigned: Optional[list[PlacesAssigned]]


Can we move these schemas into the schema file?

Sepehr-Sobhani · 2025-01-22T21:55:48Z

bc_obps/service/contact_service_v2.py

+    @classmethod
+    def get_if_authorized_v2(cls, user_guid: UUID, contact_id: int) -> Optional[Contact]:
+        user = UserDataAccessService.get_by_guid(user_guid)
+        user_contacts = ContactDataAccessService.get_all_contacts_for_user(user)
+        contact = user_contacts.filter(id=contact_id).first()
+        if user.is_industry_user() and not contact:
+            raise Exception(UNAUTHORIZED_MESSAGE)
+        return contact


I know this is the second version of a method I wrote earlier (which was never used lol). However, I'm struggling to grasp its purpose. Since we’re already retrieving all the contacts associated with the user’s operator, why do we need to check if the user is an industry user and if there’s no associated contact, why do we raise UNAUTHORIZED_MESSAGE? 🤔

Uh... good question. Maybe it was something about how reg1 had senior officers as part of the operator approval? I'll nix it for v2

Sepehr-Sobhani · 2025-01-22T22:02:43Z

bc_obps/registration/api/v2/_contacts/contact_id.py

@@ -23,7 +24,7 @@
 )
 @handle_http_errors()
 def get_contact(request: HttpRequest, contact_id: int) -> Tuple[Literal[200], Optional[ContactWithPlacesAssigned]]:
-    return 200, ContactService.get_with_places_assigned(get_current_user_guid(request), contact_id)
+    return 200, ContactServiceV2.get_with_places_assigned_v2(get_current_user_guid(request), contact_id)


Should we use ContactWithPlacesAssigned as the schema for the response? (line 18)

Sepehr-Sobhani · 2025-01-22T22:06:11Z

bc_obps/service/operation_service.py

+            (
+                Q(bc_obps_regulated_operation__id__icontains=bc_obps_regulated_operation)
+                if bc_obps_regulated_operation
+                else Q()
+            ),


Is this just a formatting change? 🤔

Yes, prettier put it in

Sepehr-Sobhani · 2025-01-22T22:48:02Z

bciers/apps/administration/app/components/operations/OperationInformationForm.tsx

+        <Note variant="important">
+          To remove the current operation representative, please select a new
+          contact to replace them.
+        </Note>


Do we need to show this message to internal users too?

Nope, thanks for catching

Sepehr-Sobhani · 2025-01-23T00:31:46Z

bciers/apps/administration/app/components/operations/OperationInformationForm.tsx

+      if (response.error.includes("Please return to Contacts")) {
+        const splitError = response.error.split("Contacts");
+        response.error = (
+          <>
+            {splitError[0]} <Link href={"/contacts"}>Contacts</Link>
+            {splitError[1]}
+          </>
+        );
+      }


nit: I think adding a comment here could be helpful.

Sepehr-Sobhani · 2025-01-23T00:45:29Z

...nistration/app/data/jsonSchema/operationInformation/administrationRegistrationInformation.ts

  const regulatedProducts: { id: number; name: string }[] =
    await getRegulatedProducts();
-
+  const contacts: {
+    items: [{ id: number; first_name: string; last_name: string }];
+  } = await getContacts();


We could use error-handling logic here! Just in case fetching data fails for regulatedProducts or contacts.

Sepehr-Sobhani · 2025-01-23T00:56:37Z

bciers/apps/administration/tests/components/operations/mocks.ts

+vi.mock("libs/actions/src/api/getContacts", () => ({
+  default: getContacts,
+}));


nit: Would it be better to move this to the contacts/mocks folder?

Sepehr-Sobhani · 2025-01-23T00:59:50Z

bciers/libs/components/src/form/fields/PlacesAssignedFieldTemplate.tsx

+  return (
+    <div className="flex min-w-full flex-col">
+      {items?.map((item) => {
+        const formData = item.children.props.formData;


nit: const { role_name, operation_name, operation_id } = item.children.props.formData;

Sepehr-Sobhani · 2025-01-23T01:04:27Z

bc_obps/registration/schema/v2/operation.py

+            'status',
+            'activities',
+            'opted_in_operation',
+            'contacts',


Do we need contacts here?!

Sepehr-Sobhani · 2025-01-23T22:12:02Z

bc_obps/registration/schema/v2/operation.py

@@ -194,7 +194,7 @@ class Meta:
            'status',
            'activities',
            'opted_in_operation',
-            'contacts',
+            # 'contacts',


Sepehr-Sobhani · 2025-01-23T22:12:34Z

bc_obps/service/contact_service_v2.py

+    def get_by_id(cls, contact_id: int) -> Contact:
+        return Contact.objects.get(id=contact_id)


We already have this in ContactDataAccessService.

BCerki commented Jan 14, 2025

View reviewed changes

BCerki force-pushed the 2524-replace-op-rep branch 5 times, most recently from fb07630 to 8583b84 Compare January 17, 2025 16:14

BCerki commented Jan 17, 2025

View reviewed changes

BCerki force-pushed the 2524-replace-op-rep branch from 8583b84 to 444fbe4 Compare January 17, 2025 16:21

BCerki marked this pull request as ready for review January 17, 2025 16:21

BCerki force-pushed the 2524-replace-op-rep branch 2 times, most recently from 5af0462 to 5bda3dc Compare January 17, 2025 22:27

Sepehr-Sobhani approved these changes Jan 23, 2025

View reviewed changes

BCerki force-pushed the 2524-replace-op-rep branch from 5bda3dc to d017894 Compare January 23, 2025 21:56

Sepehr-Sobhani approved these changes Jan 23, 2025

View reviewed changes

BCerki added 13 commits January 23, 2025 15:23

chore: BE work on replacing operation rep

403eda9

test: pytests for removing operation rep

d2ae65e

chore: update rjsf schemas

91caeaa

chore: create PlacesAssignedFieldTemplate

06bfca7

feat: replace operation rep

2366369

test: vitests for replacing operation rep

bc4cdb6

chore: update mock data

7c4d12f

chore: add link to contacts address error

2616ff1

chore: users cannot update unregistered operations

43d2894

chore: cleanup

0506322

chore: update mock data

871d314

chore: apply suggestions from code review

882d900

chore: apply more suggestions from code review

5d79839

BCerki force-pushed the 2524-replace-op-rep branch from 41e2957 to 5d79839 Compare January 23, 2025 23:33

BCerki merged commit 05ae1f7 into develop Jan 23, 2025
42 checks passed

BCerki deleted the 2524-replace-op-rep branch January 23, 2025 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace operation rep #2679

Replace operation rep #2679

BCerki commented Jan 13, 2025 •

edited

Loading

BCerki Jan 14, 2025

BCerki Jan 14, 2025

Sepehr-Sobhani Jan 23, 2025

BCerki Jan 23, 2025

BCerki Jan 17, 2025

Sepehr-Sobhani left a comment

Sepehr-Sobhani Jan 22, 2025

Sepehr-Sobhani Jan 22, 2025

BCerki Jan 23, 2025

Sepehr-Sobhani Jan 22, 2025

Sepehr-Sobhani Jan 22, 2025

BCerki Jan 23, 2025

Sepehr-Sobhani Jan 22, 2025

BCerki Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

Sepehr-Sobhani Jan 23, 2025

BCerki Jan 23, 2025

		def get_by_id(cls, contact_id: int) -> Contact:
		return Contact.objects.get(id=contact_id)

Replace operation rep #2679

Replace operation rep #2679

Conversation

BCerki commented Jan 13, 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

Sepehr-Sobhani left a 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

Choose a reason for hiding this comment

BCerki commented Jan 13, 2025 •

edited

Loading