Skip to content

33224 business-registry-model: Add additional currency serialization#4328

Merged
loneil merged 2 commits into
bcgov:mainfrom
loneil:33224ModelTweaksForAdditonalCurrency
Apr 28, 2026
Merged

33224 business-registry-model: Add additional currency serialization#4328
loneil merged 2 commits into
bcgov:mainfrom
loneil:33224ModelTweaksForAdditonalCurrency

Conversation

@loneil
Copy link
Copy Markdown
Collaborator

@loneil loneil commented Apr 27, 2026

Issue #: /bcgov/entity#33224

Description of changes:
Match the changes made to the legal-api models for currency_additional. Some were never made in this business-registry-model duplicate.

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

Signed-off-by: Lucas <lucasoneil@gmail.com>
Copilot AI review requested due to automatic review settings April 27, 2026 20:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the business-registry-model share class serialization and persistence behavior to align with legal-api’s handling of currency_additional (serialized as currencyAdditional), and adds/updates model tests to cover the new behavior.

Changes:

  • Include currencyAdditional in ShareClass.json output.
  • Add a ShareClass SQLAlchemy listener rule to clear currency_additional whenever currency is not OTHER.
  • Update and extend unit tests for share class/series JSON and listener behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
python/common/business-registry-model/src/business_model/models/share_class.py Adds currencyAdditional to JSON and normalizes currency_additional via the before_insert/before_update listener.
python/common/business-registry-model/tests/models/test_share_class.py Updates JSON expectation and adds explicit tests for currencyAdditional serialization and listener clearing/preservation rules.
python/common/business-registry-model/tests/models/test_share_series.py Updates JSON expectation to include currencyAdditional in nested share class JSON.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Lucas <lucasoneil@gmail.com>
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
30.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@loneil loneil changed the title 33224: Add additional currency serialization 33224 business-registry-model: Add additional currency serialization Apr 28, 2026
Copy link
Copy Markdown
Collaborator

@kialj876 kialj876 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

readme = "README.md"
requires-python = ">=3.13,<3.14"
dependencies = [
"sql-versioning @ git+https://github.com/bcgov/lear.git@main#subdirectory=python/common/sql-versioning-alt",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just merged Anish's dependency update PR for sql-versioning so we may need to specify the version of this package if the new one causes any issues

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

K, will merge this and see if there's any build problem from that when importing into a reliant service

@loneil loneil merged commit c2aae3d into bcgov:main Apr 28, 2026
7 of 8 checks passed
@loneil loneil deleted the 33224ModelTweaksForAdditonalCurrency branch April 28, 2026 16:09
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