Skip to content
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

21760 stage 1 overdue ARs notification #2823

Closed
wants to merge 13 commits into from
Closed

21760 stage 1 overdue ARs notification #2823

wants to merge 13 commits into from

Conversation

kzdev420
Copy link
Collaborator

Issue #: /bcgov/entity#21760

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the lear license (Apache 2.0).

@kzdev420 kzdev420 self-assigned this Jul 11, 2024
@kzdev420 kzdev420 removed the request for review from Jxio July 12, 2024 22:35
Comment on lines 65 to 72
if etype == 'bc.registry.dissolution':
identifier = email_msg.get('identifier', None)
furnishing_id = email_msg['data']['furnishing']['furnishingId']
business = ar_overdue_stage_1_notification.get_business_by_furnishing_id(furnishing_id)
if business.legal_type in ['BC', 'ULC', 'CC', 'BEN']:
source = email_msg.get('source', None)
return create_message_context_properties(etype, message_id, source, identifier, False)
return create_message_context_properties(etype, None, None, None, False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required as we are not sending the queue message to the emailer from the filer.

When it's sent from the filer, we are using the already existing format which does not provide a unique "id" that can be used. This why everything except etype == "bc.registry.names.request" is constructing a unique id based off of the incoming properties of a queue message.

We can just extend the check above for if etype == 'bc.registry.names.request': to include a check for bc.registry.dissolution

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 187 to 192
# Update corresponding furnishings entry as PROCESSED
etype = email_msg.get('type', None)
if etype and etype == 'bc.registry.dissolution':
furnishing_id = email_msg['data']['furnishing']['furnishingId']
ar_overdue_stage_1_notification.update_furnishing_status(furnishing_id,
Furnishing.FurnishingStatus.PROCESSED)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be part of tracker code.

it should take place in the processor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to detect the HTTP status of notify API in the Processor. 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename this processor to involuntary_dissolution_stage_1_notification.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename file to INVOL-DIS-STAGE-1.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 200 to 206
# Update corresponding furnishings entry as FAILED
etype = email_msg.get('type', None)
if etype and etype == 'bc.registry.dissolution':
furnishing_id = email_msg['data']['furnishing']['furnishingId']
ar_overdue_stage_1_notification.update_furnishing_status(furnishing_id,
Furnishing.FurnishingStatus.FAILED)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be part of tracker code.

should take place in processor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to detect the HTTP status of notify API in the Processor. 😢
So, I implemented this feature in update_message_tracker

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you can do is add a try/catch block around send_email in worker.py

image

If there is no exception thrown, you can assume it sent the email successfully.
if an error is thrown then you can mark it as failed and re-throw the error so the original error handling logic can run.

In both instances, maybe a new function named something like involuntary_dissolution_stage_1_processor.post_process can be called

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@@ -142,6 +143,9 @@ def process_email(email_msg: dict, flask_app: Flask): # pylint: disable=too-man
elif etype and etype == 'bc.registry.bnmove':
email = bn_notification.process_bn_move(email_msg, token)
send_email(email, token)
elif etype and etype == 'bc.registry.dissolution':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a check to confirm the data.furnishingName is one of the following:

  • DISSOLUTION_COMMENCEMENT_NO_AR
  • DISSOLUTION_COMMENCEMENT_NO_TR
  • DISSOLUTION_COMMENCEMENT_NO_AR_XPRO
  • DISSOLUTION_COMMENCEMENT_NO_TR_XPRO

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

# run worker
with patch.object(AccountService, 'get_bearer_token', return_value=token):
with patch.object(worker, 'send_email', return_value='success') as mock_send_email:
with patch.object(ar_overdue_stage_1_notification, 'get_recipient_from_auth', return_value='[email protected]'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think this is required anymore if we get the email recipient from the furnishings record

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

'furnishing': {
'type': 'PROCESSING',
'furnishingId': furnishing.id,
'furnishingName': furnishing.furnishing_name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please verify, the processor only processes the four furnishing types i mentioned in a comment above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

# get business
furnishing_id = email_info['data']['furnishing']['furnishingId']
furnishing = Furnishing.find_by_id(furnishing_id)
business = get_business_by_furnishing_id(furnishing_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a separate PR and add a relationship to the furnishing model so you can just access the business using furnishing.business.

@kzdev420 kzdev420 closed this by deleting the head repository Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants