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

15603 - Initial changes for adding in displayName for legalName changes (SP/GP) #2690

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

seeker25
Copy link
Collaborator

@seeker25 seeker25 commented Jan 25, 2024

Issue #:
bcgov/entity#15603

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 sbc-auth license (Apache 2.0).

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 260 lines in your changes are missing coverage. Please review.

Comparison is base (261dc17) 80.71% compared to head (08f2f69) 89.43%.
Report is 194 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2690      +/-   ##
==========================================
+ Coverage   80.71%   89.43%   +8.72%     
==========================================
  Files         322      179     -143     
  Lines       11915     9503    -2412     
  Branches      618       49     -569     
==========================================
- Hits         9617     8499    -1118     
+ Misses       2289      991    -1298     
- Partials        9       13       +4     
Flag Coverage Δ
accountmailerqueue 84.32% <ø> (-3.60%) ⬇️
activityloglistenerqueue ∅ <ø> (∅)
authapi 89.85% <89.14%> (-0.11%) ⬇️
authweb ∅ <ø> (∅)
eventlistenerqueue ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
auth-api/src/auth_api/__init__.py 85.33% <100.00%> (ø)
auth-api/src/auth_api/config.py 99.42% <100.00%> (+0.03%) ⬆️
auth-api/src/auth_api/exceptions/errors.py 100.00% <100.00%> (ø)
auth-api/src/auth_api/models/affiliation.py 100.00% <100.00%> (+7.31%) ⬆️
auth-api/src/auth_api/models/dataclass.py 100.00% <100.00%> (ø)
auth-api/src/auth_api/models/membership.py 72.41% <100.00%> (ø)
auth-api/src/auth_api/models/org.py 93.56% <100.00%> (+0.15%) ⬆️
auth-api/src/auth_api/models/product_code.py 96.66% <100.00%> (+0.11%) ⬆️
...th-api/src/auth_api/models/product_subscription.py 100.00% <ø> (ø)
auth-api/src/auth_api/resources/__init__.py 100.00% <100.00%> (ø)
... and 53 more

... and 151 files with indirect coverage changes

@seeker25
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-account-dev--pr-2690-jah43a31.web.app

@seeker25 seeker25 marked this pull request as ready for review January 25, 2024 21:48
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -392,12 +393,23 @@ def fix_stale_affiliations(org_id: int, entity_details: Dict, environment: str =

current_app.logger.debug('>fix_stale_affiliations')

@staticmethod
def _affiliation_details_url(affiliation: AffiliationModel) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to move this out of the model, because of circular deps, can't call model -> service, but service -> service is fine.

@@ -428,8 +440,7 @@ def sort_key(item):
raise ServiceUnavailableException('Failed to get affiliation details') from err

@staticmethod
def _combine_affiliation_details(details):
"""Parse affiliation details responses and combine draft entities with NRs if applicable."""
def _group_details(details):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactor to make this easier to deal with

return name_requests, businesses, drafts

@staticmethod
def _update_draft_type_for_amalgamation_nr(business):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactor to make this easier to deal with


@staticmethod
def _combine_nrs(name_requests, businesses, drafts):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactor to make this easier to deal with

@@ -471,6 +489,12 @@ def _combine_affiliation_details(details):

return [name_request for nr_num, name_request in name_requests.items()] + drafts + businesses

@staticmethod
def _combine_affiliation_details(details):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactor to make this easier to deal with

@@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
"""Manage the Feature Flags initialization, setup and service."""
import logging
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LD configuration setup the same as PAY now. so they're consistent. It was never used in auth.

Copy link
Collaborator

@ochiu ochiu left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -48,12 +48,24 @@ export const useBusinessStore = defineStore('business', () => {
return useOrgStore().currentOrganization
})

function determineDisplayName (resp: AffiliationResponse): string {
if (!LaunchDarklyService.getFlag(LDFlags.AlternateNamesMbr, false)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove this after we get rid of feature flag

Copy link
Collaborator

@Jxio Jxio left a comment

Choose a reason for hiding this comment

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

LGTM!

@seeker25 seeker25 merged commit 8815770 into bcgov:main Jan 25, 2024
27 of 28 checks passed
@seeker25 seeker25 changed the title 15603 - Initial changes for adding in displayName for legalName changes (SP/GP). 15603 - Initial changes for adding in displayName for legalName changes (SP/GP) Jan 25, 2024
seeker25 added a commit to seeker25/sbc-auth that referenced this pull request Jan 25, 2024
Minor fix

Small tweak, code complexity

Fix unit test

re-do feature flag

Fix LD flags

Remove updating business name in entities table, we no longer use this field. It comes from LEAR. Rewire loadBusiness to query lear, compute the business name off of LEAR's results

15603 - Initial changes for adding in displayName for legalName changes (SP/GP). (bcgov#2690)

* Initial changes for adding in displayName for legalName changes (SP/GP).

* Minor fix

* Small tweak, code complexity

* Fix unit test

* re-do feature flag

* Fix LD flags
seeker25 added a commit that referenced this pull request Jan 26, 2024
…e for SPs and GPs (#2691)

* Initial changes for adding in displayName for legalName changes (SP/GP).

Minor fix

Small tweak, code complexity

Fix unit test

re-do feature flag

Fix LD flags

Remove updating business name in entities table, we no longer use this field. It comes from LEAR. Rewire loadBusiness to query lear, compute the business name off of LEAR's results

15603 - Initial changes for adding in displayName for legalName changes (SP/GP). (#2690)

* Initial changes for adding in displayName for legalName changes (SP/GP).

* Minor fix

* Small tweak, code complexity

* Fix unit test

* re-do feature flag

* Fix LD flags

* change type to corpTypes
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