-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling changing registration purpose #2436
base: develop
Are you sure you want to change the base?
Conversation
a862acb
to
4735cb1
Compare
4735cb1
to
40d36f0
Compare
ae804fb
to
491151c
Compare
491151c
to
4541316
Compare
ec92785
to
b082037
Compare
b082037
to
e5b2fe5
Compare
…n multiples are required; fixed bug where reporting ops were also being marked as regulated erroneously
… of changing reg purpose - more refinement likely needed. Lots of failures
…O as original RP bc of doc count that's haunting me
330dd49
to
ddf6d7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking on this monster, Andrea! If you want to go over anything, let me know--I ~think I followed things ok, but I may have made irrelevant comments where my brain couldn't keep up with yours
lambda: not ( | ||
operation.registration_purpose != Operation.Purposes.ELECTRICITY_IMPORT_OPERATION | ||
and operation.documents.filter(Q(type__name='process_flow_diagram') | Q(type__name='boundary_map')) | ||
.distinct() | ||
.count() | ||
< 2 | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yikes, was this check backwards before?
if opted_in_detail and original_operation.status == Operation.Statuses.REGISTERED: | ||
opted_in_detail.set_archive(user_guid) | ||
elif opted_in_detail: | ||
opted_in_detail.delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should archive_or_delete_opted_in_operation_detail
or remove_opted_in_operation_detail
be used here?
@@ -459,3 +523,40 @@ def update_operator(cls, user_guid: UUID, operation: Operation, operator_id: UUI | |||
operation.save(update_fields=["operator_id"]) | |||
operation.set_create_or_update(user_guid) | |||
return operation | |||
|
|||
@classmethod | |||
def handle_change_of_registration_purpose( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding a docstring and maybe some inline since changing registration purpose is so complicated. I'll mark up below where I had trouble following at first
user_guid, original_operation.id, 'new_entrant_application' | ||
) | ||
|
||
if payload.registration_purpose == Operation.Purposes.ELECTRICITY_IMPORT_OPERATION: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and onward stuff is getting deleted to accommodate the new purpose. Again, maybe a new_purpose
variable would help, or a comment. Stuff is getting deleted throughout the whole function, but not always for the same reason, and I found that confusing
opted_in_detail.set_archive(user_guid) | ||
elif opted_in_detail: | ||
opted_in_detail.delete() | ||
elif original_operation.registration_purpose == Operation.Purposes.NEW_ENTRANT_OPERATION: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For opt-in and new entrant, this function looks at the old purpose and deletes stuff that only applied to the old purpose. Maybe something like old_purpose = operation.registration_purpose
up at the beginning of the function? It's not the operation that's original
, we're always talking about the same one.
assert self.operation.updated_by == self.user | ||
assert self.operation.updated_at > self.operation.created_at | ||
|
||
match original_purpose: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with a reorg like I suggested above (or a different, better one you come up with), I think we can avoid the match/case and the test will be easier to read
if new_purpose == Operation.Purposes.OPTED_IN_OPERATION: | ||
self._set_opted_in_operation_detail() | ||
elif new_purpose == Operation.Purposes.NEW_ENTRANT_OPERATION: | ||
self._set_new_entrant_info() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm I understand how this would work in the app to make sure this test is representative. Say I'm on the operation page and click edit. I change the registration purpose, and at that point the opt-in/new entrant fields pop up. If I try to submit, I won't be able to until I fill in those fields. Correct? (Vs. how it is in the registration workflow where purpose and opt-in/new entrant are in separate steps and separate API calls.)
def _create_new_entrant_payload(self): | ||
return {"date_of_first_shipment": "On or after April 1, 2024", "new_entrant_application": MOCK_DATA_URL} | ||
|
||
def _prepare_test_data(self, registration_purpose, type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced we need this function or _set_operation_information. I think we could achieve the same setup with baker recipes
from registration.utils import custom_reverse_lazy | ||
|
||
|
||
class TestChangingRegistrationPurpose(CommonTestSetup): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thoroughness of this test is so beautiful 🥹
@@ -174,7 +199,14 @@ def create_or_update_operation_v2( | |||
operation_id: UUID | None = None, | |||
) -> Operation: | |||
user_operator: UserOperator = UserDataAccessService.get_user_operator_by_user(user_guid) | |||
# will need to retrieve operation as it exists currently in DB first, to determine whether there's been a change to the RP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so this ended up being way less scary-looking than I anticipated, thank you!
Issue #2169
Note that this is only for the backend changes required. The frontend functionality to allow for changing registration purpose after the operation has already registered will follow in a subsequent PR.
test_changing_registration_purposes.py
file that tests for every possible scenario of changing registration purpose from A to B, for both operation types (LFO and SFO). Also accounts for whether the operation has a status of "Registered" (in which case no-longer-relevant database records will be archived) or not registered (in which case no-longer-relevant records are deleted).