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

Cherry Picked all new commits from main to the feature branch #2499

Merged
merged 101 commits into from
Mar 22, 2024

Conversation

JazzarKarim
Copy link
Collaborator

@JazzarKarim JazzarKarim commented Mar 7, 2024

Issue #: /bcgov/entity#19451

Description of changes:
Cherry Picked all new commits from main to the feature branch

No changes?? (remove amalgamating information section for TING (#2407))
Left this out for now: 18803 Add amalgamation tracker (#2426) (deals with messages in queue)
Not sure if i need to change something here #2444
Not sure what to do in business filings as the file has changed https://github.com/bcgov/lear/pull/2478/files#diff-3726ee4258cac46a611a4f1aee86c82be813913b2f6dcff918ac71b33aac8db5

After this PR, the feature branch should be up-to-date with all the latest changes of main.

I tried as much as possible to find/fix everything. It's impossible for me to find everything though so some stuff might be broken in case I wrongly fixed a conflict. Should be small stuff though I hope.

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

@JazzarKarim JazzarKarim self-assigned this Mar 7, 2024
@JazzarKarim JazzarKarim marked this pull request as ready for review March 7, 2024 19:03
@JazzarKarim JazzarKarim marked this pull request as draft March 7, 2024 19:04
Copy link

sonarcloud bot commented Mar 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@JazzarKarim JazzarKarim marked this pull request as ready for review March 20, 2024 22:21
vysakh-menon-aot and others added 19 commits March 20, 2024 15:40
* AmalTED pdf

* Amalgamating pdf

* remove Dissolution/Restoration Information section

* fix lint error

* lint with flake8

* lint
* 19154

Signed-off-by: Hongjing Chen <Hongjing.Chen@gov.bc.ca>

* fix linting

Signed-off-by: Hongjing Chen <Hongjing.Chen@gov.bc.ca>

* update error messages

Signed-off-by: Hongjing Chen <Hongjing.Chen@gov.bc.ca>

---------

Signed-off-by: Hongjing Chen <Hongjing.Chen@gov.bc.ca>
* fix spacing and TING

* retrive to the previous output
* added first draft of outputs

* testing adding folder

* added draft amalgamation outputs

* cleaned up application syntax

* fixed typo

* added data formatting and noa

* added dates for certificate of amalgamation

* repeated effective_date_time in business details for amalgamation application

* fixed lint issue

* fixed typo

* fixed notice of articles and removed amalgamation/nameRequest

* removed amalgamations.html from business-summary.html

* reestablished amalgamations.html

* removed completingParty from amalgamations and added from incorp

* removed incorp/bencompany from noa in amalgamationApp
* 18803 add amalgamation email

* fix small

* fix lint issue

* fix small

* fix the get_filing parts

* fix small

* fix the test

* fix lint issue

* fix lint issue

* update version
from business_model import AmalgamatingBusiness, Amalgamation, Document, Filing, LegalEntity, RegistrationBootstrap, db
from entity_queue_common.service_utils import QueueException
from legal_api import db
from legal_api.models import (
Copy link
Collaborator

Choose a reason for hiding this comment

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

The filer imports it's models from the business_model library, not from legal_api. This can also be updated for the filer in tests/unit/__init__.py and test_amalgamation_application.py.

Also, you can probably remove the AccountService import from line 32 in this file since it isn't used.

from flask_babel import _ as babel # noqa: N813, I004, I001; importing camelcase '_' as a name

# noqa: I003
from legal_api.errors import Error
from legal_api.models import ConsentContinuationOut
from legal_api.services.filings.validations.common_validations import validate_court_order
from legal_api.services.filings.validations.continuation_out import validate_foreign_jurisdiction
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be imported from common_validations in the line above since at the moment this is a circular import.


# revision identifiers, used by Alembic.
revision = 'ba862cfda9e5'
down_revision = 'f978e34aa8bb'
Copy link
Collaborator

@leodube-aot leodube-aot Mar 20, 2024

Choose a reason for hiding this comment

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

This down_revision doesn't exist, throwing a KeyError when trying to run the unit tests. I haven't worked much in the migration scripts but this seems to indicate we are missing one of the migrations?

Copy link
Collaborator

@argush3 argush3 Mar 21, 2024

Choose a reason for hiding this comment

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

We don't need to bring over any of the new migration scripts directly most of the time. We typically cut new tickets to incorporate the new db updates coming in from main.

In the legal name branch, we have mostly collapsed all the alembic script updates into a few scripts.

@JazzarKarim you can just leave these out as I believe we have already ported over these changes. Can you double check that https://github.com/bcgov/lear/blob/feature-legal-name/legal-api/migrations/versions/004f7558063f_second.py in legal name branch have the changes in both of the scripts you cherry picked over for this PR? If things check out, we can remove these two scripts from the PR.

And thanks for flagging this @leodube-aot !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed both scripts.

I confirmed that these changes were already in the legal name branch but in this file: https://github.com/bcgov/lear/blob/feature-legal-name/legal-api/migrations/versions/5238dd8fb805_.py

So, I believe it should be OK to remove them. Thanks for catching that Leo and Argus!

Copy link

sonarcloud bot commented Mar 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

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.

nice work. thanks for doing this big sync!

Copy link
Collaborator

@chenhongjing chenhongjing left a comment

Choose a reason for hiding this comment

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

Great work Karim 👍 😺

Copy link
Collaborator

@leodube-aot leodube-aot 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 as well!

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

@JazzarKarim JazzarKarim merged commit 9622851 into bcgov:feature-legal-name Mar 22, 2024
20 of 28 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.