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

20360 Update reg and change of reg outputs with alt name #2538

Merged
merged 14 commits into from
Apr 17, 2024
Merged

20360 Update reg and change of reg outputs with alt name #2538

merged 14 commits into from
Apr 17, 2024

Conversation

kzdev420
Copy link
Collaborator

@kzdev420 kzdev420 commented Mar 22, 2024

Issue #: /bcgov/entity#20360

Description of changes:

Screenshot 2024-03-22 at 11 26 24 AM Screenshot 2024-03-22 at 11 26 49 AM

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

@kzdev420 kzdev420 requested a review from argush3 March 22, 2024 18:24
@kzdev420 kzdev420 self-assigned this Mar 22, 2024
@argush3
Copy link
Collaborator

argush3 commented Mar 22, 2024

So have you tested registration and change of registration outputs against GPs and the different flavors of SP to see if they load? Can you give me a summary of scenarios you've tested?

Also, for change of registration, did you verify the amended registration statement output?
image

Are you comparing OCP output against GCP output? Because in your registration sample output, the "business name" is missing.

@argush3
Copy link
Collaborator

argush3 commented Mar 22, 2024

You'll need to rebase against legal name branch as bunch of new changes have been merged.

@kzdev420 kzdev420 marked this pull request as ready for review March 27, 2024 15:58
@kzdev420 kzdev420 requested a review from JazzarKarim April 2, 2024 15:27
@kzdev420 kzdev420 requested review from tshyun24 and argush3 April 2, 2024 16:55
@argush3
Copy link
Collaborator

argush3 commented Apr 4, 2024

so I had already asked above but never got my comment addressed.

I need the following from you:

  • A list of OCP vs GCP outputs for the scenarios you tested
  • Did you verify the amended registration statement output?
  • The registration output you attached in the description has no value for "business name". Does your most recent code provide that now? If so, please provided updated OCP vs GCP outputs

Copy link
Collaborator

@JazzarKarim JazzarKarim left a comment

Choose a reason for hiding this comment

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

From what I can see, LGTM as long as Argus' comments get resolved 👍

@@ -903,6 +901,8 @@ def _format_change_of_registration_data(
prev_party_json = VersionedBusinessDetailsService.party_revision_json(
prev_completed_filing, prev_party, True
)
# current_party type is person, but prev_party type is organization
# so has issue at _has_party_name_change()
if self._has_party_name_change(prev_party_json, party):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FM1064603, filing_id: 148571:
In this case, prev_party is organization. current_party is person.
So _has_party_name_change function returns the error. We should fix this logic

Copy link
Collaborator

@argush3 argush3 Apr 12, 2024

Choose a reason for hiding this comment

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

We can fix this in another ticket. We need to think about this more when we get back to this work.

I'll create a new ticket.

Can you find a business to test with that doesn't have party type changing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am trying to find it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ignore my previous comment. This is a code issue, not a data issue I think.

You are pulling back an AN record when the code calls get_party_revision.

image

It should be pulling back the LE person history record.

image

I'm assuming this is related to your _has_party_name_change error.

Copy link
Collaborator Author

@kzdev420 kzdev420 Apr 12, 2024

Choose a reason for hiding this comment

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

Okay, if business.legal_entity_id is null and colin_entity_id is True, then we should pull back the Colin_entity history?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, but i'm not entirely sure if that will work as is. you may need to do some work to get it working. maybe put a todo and ignore that scenario for now. just say we need to still take care of that scenario in the future.

addresses_result = addresses_current.union(addresses_historical).all()
for address_result in addresses_result:
address, _ = address_result
addresses = addresses_current.union(addresses_historical).all()
Copy link
Collaborator Author

@kzdev420 kzdev420 Apr 12, 2024

Choose a reason for hiding this comment

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

FM1000406, filing_id: 146925
In this case, business_id is 691 ( alternate_name_id )

But, there are no records for this business in Address and Address_history table ( my local DB )
So, it returns None, and makes the error

)

party_version = history_cls(LegalEntity)
columns_to_select = VersionedBusinessDetailsService.select_revision_columns(party_version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if you are selecting the right columns for the party in all cases. For example, it looks like if it's a SP person, SP DBA LEAR or SP DBA COLIN, you are selecting the alternate name columns. I think you should be selecting the columns for the LE or AN record.

@argush3
Copy link
Collaborator

argush3 commented Apr 12, 2024

Just tested your PR against FM1064603 for the statement of registration output. It's still missing the business name. I've mentioned this a few times now...

image

@kzdev420
Copy link
Collaborator Author

Just tested your PR against FM1064603 for the statement of registration output. It's still missing the business name. I've mentioned this a few times now...

image

Sorry, I forgot to push the changes for this. Just pushed

…into 20360_update_reg_and_change_of_reg_outputs_with_alt_name
@kzdev420 kzdev420 requested review from JazzarKarim and argush3 April 17, 2024 15:36
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
2.3% Duplication on New Code

See analysis details on SonarCloud

@@ -572,7 +572,7 @@ def party_role_revision_json(filing, party_role_revision, is_ia_or_after) -> dic
def get_party_revision(filing, party_id) -> dict:
"""Consolidates all party changes up to the given transaction id."""
business = BusinessService.fetch_business_by_filing(filing)
if filing.legal_entity_id:
if filing.legal_entity_id or business.legal_entity_id:
Copy link
Collaborator

@argush3 argush3 Apr 17, 2024

Choose a reason for hiding this comment

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

I'm not sure about this change and how you are grabbing party revision data in general for all the various scenarios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FM1064603, filing_id: 148571.

In this case, the filing doesn't have the legal_entity. it has the alternate_name_entity.
And alternate_name_entity has the legal_entity.

I think you mentioned that filing pull back the legal_entity in this case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I mentioned that. But I think the logic needs to be more precise. Anyways, we can pick this up in the new ticket.

Copy link
Collaborator

@argush3 argush3 left a comment

Choose a reason for hiding this comment

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

Approving. As per our conversation, outstanding issues that still need to be addressed will be picked up in #20903

@kzdev420 kzdev420 merged commit e7d04a5 into bcgov:feature-legal-name Apr 17, 2024
4 of 5 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.

3 participants