Chore: 2685 operation representative address can not be deleted #3052

Sepehr-Sobhani · 2025-03-19T20:36:15Z

BCerki

Great work on this, looks very thorough! I had a couple questions about triggers because I haven't used them before.

For manual testing, I turned off the required fields in the FE and tried to submit missing addresses bits and got the following:

I don't think your PR introduced this, but when editing an existing contact there's a problem with the error clearing: https://www.loom.com/share/aed330b2cd9342b0b242438d0cb0e4fb
I was able to create an new contact with role operation rep without address info:
.
If I try to assign a contact without an address as a op rep (with rjsf required turned off), I get this:
(a user would have a really hard time getting here, probably not worth addressing

BCerki · 2025-03-27T21:16:54Z

bc_obps/registration/models/address.py

+                    if exists (
+                        select 1
+                        from erc.contact
+                        where address_id = new.id


Is new something that comes from pgtrigger? The docs talk about OLD and NEW but I didn't see an example like this

Yes, new is from PostgreSQL’s trigger system, used by pgtrigger. In a BEFORE UPDATE trigger, new is the proposed row data.
https://www.postgresql.org/docs/current/plpgsql-trigger.html#PLPGSQL-DML-TRIGGER-NEW

BCerki · 2025-03-27T23:25:48Z

bc_obps/registration/models/contact.py

+                when=pgtrigger.Before,  # Trigger before the update
+                operation=pgtrigger.Update,  # Only for UPDATE operations
+                condition=pgtrigger.Q(
+                    old__address__isnull=False,  # Old address was not null


I think we'd want to prevent a null new address even if the old address was null. Unless this is to accommodate some incomplete imported data?

Good catch!

After implementing this, I noticed that removing the old__address__isnull=False condition causes an issue. If a contact already exists in the database and the user selects it in the OP REP step of registration, but the contact has no address, the user is prevented from filling out the address fields. This happens because we update the contact fields first, and by the time we process the address, an error occurs.

BCerki · 2025-03-27T23:27:45Z

bc_obps/registration/models/contact.py

+                    new__address__isnull=True,  # New address is null
+                ),
+                func="""
+                    if new.business_role_id = 'Operation Representative'


This comment may be irrelevant once I understand how new works, but if the old role was operation rep and we didn't supply a new role because nothing changed, will this still block setting an address to null?

if new.business_role_id is 'Operation Representative' (either unchanged or explicitly set), the func will raise the exception.

BCerki · 2025-03-28T15:07:09Z

bc_obps/registration/tests/endpoints/_contacts/test_contact_id.py

+        with patch('registration.models.BusinessRole.objects.get', return_value=senior_officer_role) as mock_get:
+            # Assert that we have only one contact(to make sure we are updating the contact and not creating a new one)
+            assert Contact.objects.count() == 1
+            # Assert that we are not creating a new address


Suggest adding to the comment where the two addresses come from. I always forget which bakers add addresses

BCerki · 2025-03-28T15:12:37Z

bc_obps/registration/tests/models/test_contact.py

+    def test_no_trigger_when_address_already_null(self):
+        """Test that updating a contact with NULL address doesn’t trigger if address stays NULL."""
+        # Update another field
+        self.null_address_contact.email = "[email protected]"
+        self.null_address_contact.save()
+
+        # Verify the update succeeded and address is still NULL
+        self.null_address_contact.refresh_from_db()
+        self.assertEqual(self.null_address_contact.email, "[email protected]")
+        self.assertIsNone(self.null_address_contact.address)


Similar to above question, do we need to allow this case? If someone somehow got away with giving us a null address, we'd probably want to them fix it

I have changed that part of the trigger so this test is no longer valid.

Test reverted 😃
#3052 (comment)

BCerki · 2025-03-28T15:13:32Z

bc_obps/service/contact_service.py

@@ -136,3 +142,12 @@ def raise_exception_if_contact_missing_address_information(cls, contact_id: int)
            raise Exception(
                f'The contact {contact.first_name} {contact.last_name} is missing address information. Please return to Contacts and fill in their address information before assigning them as an Operation Representative here.'
            )
+
+    @classmethod
+    def validate_operation_representative_address(cls, contact_id: int, address_data: DictStrAny) -> None:


start function name with _?

Sepehr-Sobhani · 2025-03-28T18:38:54Z

I don't think your PR introduced this, but when editing an existing contact there's a problem with the error clearing: https://www.loom.com/share/aed330b2cd9342b0b242438d0cb0e4fb

That contact is an Operation Representative, so we must prevent the user from clearing address fields.

Sepehr-Sobhani · 2025-03-28T18:42:50Z

I was able to create an new contact with role operation rep without address info:
.

You can create an OP REP without an address, but you cannot delete the address or remove address fields. This is because the contact instance is created first, and the address is assigned afterward (FK relationship). Preventing users from creating the contact isn't an option, as many other contact types don't require an address.

Sepehr-Sobhani · 2025-03-28T20:24:14Z

If I try to assign a contact without an address as a op rep (with rjsf required turned off), I get this:
(a user would have a really hard time getting here, probably not worth addressing

This is correct because schema validation prevents the user from doing that. However, we currently don't have a way to display backend schema validation errors since we make fields required in RJSF.
I'm going to make a small tweak to show the first error when a validation error occurs. I don't expect this to happen often, but we can improve it later if we need to display multiple errors at once.

BCerki · 2025-03-28T22:35:21Z

I don't think your PR introduced this, but when editing an existing contact there's a problem with the error clearing: https://www.loom.com/share/aed330b2cd9342b0b242438d0cb0e4fb

That contact is an Operation Representative, so we must prevent the user from clearing address fields.

Yes! I mean that usually when a user triggers an error, fixes it, and then resubmits, the error disappears on successful submission. On the contact page, the error doesn't disappear until you refresh.

Sepehr-Sobhani · 2025-03-28T22:44:50Z

I don't think your PR introduced this, but when editing an existing contact there's a problem with the error clearing: https://www.loom.com/share/aed330b2cd9342b0b242438d0cb0e4fb

That contact is an Operation Representative, so we must prevent the user from clearing address fields.

Yes! I mean that usually a user triggers an error, fixes it, and then resubmits, the error disappears on successful submission. On the contact page, the error doesn't disappear until you refresh.

😮

BCerki

If you want to address the error message refresh in this PR, I can look again, otherwise it can be a separate bug ticket and this is good to go

Sepehr-Sobhani · 2025-03-28T22:51:46Z

If you want to address the error message refresh in this PR, I can look again, otherwise it can be a separate bug ticket and this is good to go

I'll try to fix it if it's a quick fix; otherwise, I'll create a bug ticket.

Sepehr-Sobhani · 2025-03-28T23:02:05Z

If you want to address the error message refresh in this PR, I can look again, otherwise it can be a separate bug ticket and this is good to go

Fixed!

…resentatives Signed-off-by: SeSo <[email protected]>

…esentatives Signed-off-by: SeSo <[email protected]>

Signed-off-by: SeSo <[email protected]>

…rors Signed-off-by: SeSo <[email protected]>

Signed-off-by: SeSo <[email protected]>

Sepehr-Sobhani force-pushed the chore/2685-operation-representative-address-can-not-be-deleted branch from a6961a8 to 6953d1c Compare March 19, 2025 23:54

pbastia force-pushed the develop branch from 6a0661a to 41ae97d Compare March 24, 2025 17:10

Sepehr-Sobhani force-pushed the develop branch from 06db73a to 4ba58ea Compare March 26, 2025 23:23

Sepehr-Sobhani force-pushed the chore/2685-operation-representative-address-can-not-be-deleted branch 2 times, most recently from 2d4ca2a to d82c4f6 Compare March 27, 2025 23:28

BCerki reviewed Mar 28, 2025

View reviewed changes

Sepehr-Sobhani force-pushed the chore/2685-operation-representative-address-can-not-be-deleted branch from 9005ea6 to 5550a4f Compare March 28, 2025 18:31

Sepehr-Sobhani force-pushed the chore/2685-operation-representative-address-can-not-be-deleted branch from 56a608a to 84ca896 Compare March 28, 2025 21:51

BCerki approved these changes Mar 28, 2025

View reviewed changes

Sepehr-Sobhani force-pushed the chore/2685-operation-representative-address-can-not-be-deleted branch from c262049 to 61c135a Compare March 28, 2025 23:01

Sepehr-Sobhani and others added 11 commits March 28, 2025 16:59

chore: add triggers to prevent NULL address updates for Operation Rep…

e67f75b

…resentatives Signed-off-by: SeSo <[email protected]>

chore: add validation for address fields when updating Operation Repr…

882558c

…esentatives Signed-off-by: SeSo <[email protected]>

test: enhance contact update tests for address validation scenarios

02fc1b4

Signed-off-by: SeSo <[email protected]>

chore: fix migration after rebase

c214999

chore: fix migrations post-rebase

3b536a1

chore: update filename

5e49681

chore: rename migration file and update dependency reference

8473d7e

Signed-off-by: SeSo <[email protected]>

chore: improve error handling and response messages for validation er…

58aeb0b

…rors Signed-off-by: SeSo <[email protected]>

chore: implement review suggestions

1957c15

Signed-off-by: SeSo <[email protected]>

chore: reset error state before form submission

ced3afa

Signed-off-by: SeSo <[email protected]>

chore: update migration name

c8afcd7

Sepehr-Sobhani force-pushed the chore/2685-operation-representative-address-can-not-be-deleted branch from 4d8df10 to c8afcd7 Compare March 28, 2025 23:59

Sepehr-Sobhani merged commit 2e04ec7 into develop Mar 29, 2025
22 checks passed

Sepehr-Sobhani deleted the chore/2685-operation-representative-address-can-not-be-deleted branch March 29, 2025 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: 2685 operation representative address can not be deleted #3052

Chore: 2685 operation representative address can not be deleted #3052

Sepehr-Sobhani commented Mar 19, 2025 •

edited

Loading

BCerki left a comment

BCerki Mar 27, 2025

Sepehr-Sobhani Mar 28, 2025

BCerki Mar 27, 2025

Sepehr-Sobhani Mar 28, 2025

Sepehr-Sobhani Mar 28, 2025

BCerki Mar 27, 2025

Sepehr-Sobhani Mar 28, 2025

BCerki Mar 28, 2025

BCerki Mar 28, 2025

Sepehr-Sobhani Mar 28, 2025

Sepehr-Sobhani Mar 28, 2025

BCerki Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

BCerki commented Mar 28, 2025 •

edited

Loading

Sepehr-Sobhani commented Mar 28, 2025

BCerki left a comment

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

Chore: 2685 operation representative address can not be deleted #3052

Chore: 2685 operation representative address can not be deleted #3052

Conversation

Sepehr-Sobhani commented Mar 19, 2025 • edited Loading

BCerki 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

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

BCerki commented Mar 28, 2025 • edited Loading

Sepehr-Sobhani commented Mar 28, 2025

BCerki left a comment

Choose a reason for hiding this comment

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 28, 2025

Sepehr-Sobhani commented Mar 19, 2025 •

edited

Loading

BCerki commented Mar 28, 2025 •

edited

Loading