-
Notifications
You must be signed in to change notification settings - Fork 71
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
23522-23562 continuation in workflow change #3026
base: main
Are you sure you want to change the base?
Conversation
legal-api/src/legal_api/resources/v2/business/business_filings/business_filings.py
Outdated
Show resolved
Hide resolved
legal-api/src/legal_api/resources/v2/business/business_filings/business_filings.py
Show resolved
Hide resolved
legal-api/src/legal_api/resources/v2/business/business_filings/business_filings.py
Show resolved
Hide resolved
legal-api/src/legal_api/resources/v2/business/business_filings/business_filings.py
Show resolved
Hide resolved
legal-api/src/legal_api/resources/v2/business/business_filings/business_documents.py
Show resolved
Hide resolved
legal-api/src/legal_api/resources/v2/business/business_filings/business_filings.py
Show resolved
Hide resolved
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.
Looks great! Lots of learning for me here :)
@@ -73,6 +73,7 @@ async def process_event(msg: Dict, flask_app: Flask): # pylint: disable=too-man | |||
with flask_app.app_context(): | |||
if msg['type'] == 'bc.registry.admin.bn': | |||
await admin.process(msg) | |||
return |
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.
What is this fixing? Please call it out in your PR description or ticket so it's recorded.
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.
This return was removed by mistake (adding it back now). It was not breaking any functionality but its adding unwanted logs in entity-bn.
is_future_effective=True) | ||
filing_name='unknownFiling', | ||
filing_status='PAID', | ||
is_future_effective=True) |
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 all the cleanup, in this file and the others.
review.save() | ||
|
||
filing.set_review_decision(Filing.Status.CHANGE_REQUESTED.value) | ||
if filing_status != Filing.Status.DRAFT.value: |
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 always prefer checking the positive conditions, since a negative condition could cause errors if, say, we added a filing status. Up to you whether to change or leave this.
legal-api/tests/unit/services/filings/validations/test_continuation_in.py
Show resolved
Hide resolved
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.
When you're happy with this then I'm happy with it, too.
You might still want a review from Karim or Argus or Hongjing.
Quality Gate passedIssues Measures |
Issue #: /bcgov/entity#23522, /bcgov/entity#23562
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).