-
Notifications
You must be signed in to change notification settings - Fork 81
DBC queue service: Invoke retry response for relevant filings not yet in complete state + use business-event format #3683
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
Conversation
Signed-off-by: Lucas <lucasoneil@gmail.com>
5e72cab to
9955d98
Compare
798ec34 to
1d03fb4
Compare
Signed-off-by: Lucas <lucasoneil@gmail.com>
00ebc0b to
cd94576
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.
Pull Request Overview
This PR refactors the business digital credentials queue service to handle filing status appropriately and adopt a business-event message format. The main purpose is to implement retry logic for filings not yet in complete state and transition from the legacy "filingMessage" format to the new business-event topic format.
- Implements retry logic using FilingStatusException for incomplete filings instead of ignoring them
- Migrates from "filingMessage" format to business-event format with specific event types
- Updates message parsing to use
filing.header.filingIdinstead offilingMessage.filingIdentifier
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| exceptions.py | Adds new FilingStatusException for retry scenarios |
| business_digital_credentials.py | Refactors message processing logic and error handling for business-event format |
| test_business_digital_credentials.py | Updates all tests to use new format and validates FilingStatusException behavior |
Comments suppressed due to low confidence (1)
queue_services/business-digital-credentials/tests/unit/resources/test_business_digital_credentials.py:329
- This test case sets filingId to None, but the actual code checks
if not filing_id:which would catch None. However, there's no test case for missing 'header' key or missing 'filing' key, which would trigger the KeyError/TypeError handling in the try-catch block on lines 148-153.
ce.data = {"filing": {"header": {"filingId": None}}}
...gital-credentials/src/business_digital_credentials/resources/business_digital_credentials.py
Show resolved
Hide resolved
...gital-credentials/src/business_digital_credentials/resources/business_digital_credentials.py
Show resolved
Hide resolved
severinbeauvais
left a comment
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.
LGTM
|
@vysakh-menon-aot , @hfekete , @TVWerdal , do you have a moment to review this PR, please? |
Issue #: /bcgov/entity#29610
Description of changes:
Change to use business-event topic to detect these filings (have set up and tried subscriber), so use a previous filing type/message format parse.
Doing a dissolution and filing message came in with draft status before going to complete state. So need to add onto the retry (exponential backoff before going to DLQ as configured in subscription) instead of just returning. See Error Code GCP pub/sub list: https://cloud.google.com/pubsub/docs/reference/error-codes
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).