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

22955 - Verify update-colin-filings job to use new db versioning #3112

Conversation

AimeeGao
Copy link
Collaborator

@AimeeGao AimeeGao commented Dec 5, 2024

Issue #: /bcgov/entity#22955

Description of changes:

  1. Investigated the job's functionality:

    • Job only handles API calls from legal-api and colin-api
    • No direct DB operations
    • No legal-api and db versioning package usage
  2. Conclusions:

    • No changes needed in db versioning setup
    • Remove the update-colin-filings flag in legal-api's flags.json
  3. Findings of db versioning relationship issue:

    • Found versioned objects (BusinessVersion) missing relationship attributes (Resolutions)
    • Root cause:
      • Our db versioning only copies table columns but misses the relationship mappings
      • When versioning is enabled, BusinessVersion lacks the resolutions relationship that exists in the original Business model
    • Solutions:
      • Added direct Resolution query to replace relationship access
      • Updated _set_shares method to handle both versioned and non-versioned cases
      • Code changes:
      # Before
      business_dates = [item.resolution_date.isoformat() for item in primary_or_holding_business.resolutions]
      
      # After
      resolutions = Resolution.get_resolution_by_business_id(primary_or_holding_business.id)
      business_dates = [item.resolution_date.isoformat() for item in resolutions]

image

  • Debug logs showing the issue:

    When the FF is ON:
    image
    image

    When the FF is OFF:
    image
    image

  1. Verification steps:

    • Tested legal-api with feature flags ON & OFF
    • Verified update-colin-filings job functionality
    • Confirmed queue message processing works correctly in the local environment
    • All unit tests passed successfully
    • Verified resolution dates are correctly retrieved in both versioned and non-versioned scenarios

ScreenShots:

  • Legal-API OFF
    • Job works successfully in the local environment

image

  • Legal-API ON
  1. Created a new FED filing (Change of Address) - Filing ID: 151265
    image

  2. Verified filing exists in the local DB
    image

  3. Updated filing status to COMPLETED for testing
    image

  4. Then run the job locally again and confirm the filing(Filing ID: 151265) was successfully updated
    image

  5. Verified passed all unit test
    image
    image

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).

@AimeeGao AimeeGao marked this pull request as draft December 5, 2024 01:31
Copy link

sonarqubecloud bot commented Dec 5, 2024

@AimeeGao AimeeGao self-assigned this Dec 5, 2024
@AimeeGao AimeeGao marked this pull request as ready for review December 5, 2024 16:40
@@ -287,7 +287,8 @@ def _set_shares(primary_or_holding_business, amalgamation_filing, transaction_id
share_classes = VersionedBusinessDetailsService.get_share_class_revision(transaction_id,
primary_or_holding_business.id)
amalgamation_filing['shareStructure'] = {'shareClasses': share_classes}
business_dates = [item.resolution_date.isoformat() for item in primary_or_holding_business.resolutions]
resolutions = Resolution.get_resolution_by_business_id(primary_or_holding_business.id)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a temporary fix to handle the missing relationship in versioned objects.
I've tested both scenarios (FF ON/OFF), and everything works fine.

But I'm not sure if this approach is acceptable.
I only found this issue in this ticket and am unsure if it exists elsewhere. (Didn't find this issue from the previous submissions of tickets)

Should we:

  1. Investigate and fix our db versioning to handle relationships properly
  2. Consider how this change might impact our already completed tickets that use versioning

@leodube-aot would appreciate your thoughts on this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this is an issue we have been aware of, but I don't think it is too prevalent in the code.

Screenshot 2024-12-05 at 9 52 09 AM

I created a ticket to investigate and fix the issue. I don’t think it is super high priority, but would be nice to have. We can use the workaround for now (and then clean it up in the new ticket). bcgov/entity#24754

Copy link
Collaborator

@kzdev420 kzdev420 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@eason-pan-bc eason-pan-bc left a comment

Choose a reason for hiding this comment

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

LGTM 🛩️
Nice workaround for the issue!

@AimeeGao AimeeGao merged commit b1fa4d2 into bcgov:feature-db-versioning Dec 5, 2024
20 checks passed
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.

4 participants