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

Gate Persistence: Legal Form should be optional #317

Conversation

SujitMBRDI
Copy link
Contributor

@SujitMBRDI SujitMBRDI commented Jul 4, 2023


title: 'Gate Persistence: Legal Form should be optional'

Description

This pull request makes column 'legal_form_id' from table 'bpdmgate.legal_entities' to accept Null values. And making Legal Form as optional attribute for Legal Entity on Gate Service.

Fixes #272

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

  • DEPENDENCIES are up to date. Dash license tool. Committers can open IP issues for restricted libs.
  • Copyright and license header are present on all affected files

Copy link
Contributor

@nicoprow nicoprow 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 so far, just needs a small change in the migration file.

Other than that the commit message format needs to be adapterd. Our commit message format is inspired by angular2 format: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit

Scopes in our commit messages are not taken from a list though but can be freely chosen by you.

Regarding this, your commit message would be something like this: "fix(gate): ..."

@@ -0,0 +1,3 @@
-- Description: Make 'legal_form_id' column optional in the 'legal_entities' table
-- Alter the 'legal_form_id' column to allow NULL values
ALTER TABLE bpdmgate.legal_entities ALTER COLUMN legal_form_id DROP NOT NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the schema name here. Without the explicit schema name we would be more flexible in the configuration on where these migration files are applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed schema name

@SujitMBRDI
Copy link
Contributor Author

Looks good so far, just needs a small change in the migration file.

Other than that the commit message format needs to be adapterd. Our commit message format is inspired by angular2 format: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit

Scopes in our commit messages are not taken from a list though but can be freely chosen by you.

Regarding this, your commit message would be something like this: "fix(gate): ..."

Thanks for providing syntax for adding commit message. I have adapted format in recent commit.
Also fixed schema name as suggested.

@SujitMBRDI SujitMBRDI requested a review from nicoprow July 7, 2023 07:34
@SujitMBRDI SujitMBRDI force-pushed the bugfix/gate-persist-legal-form-optional branch 2 times, most recently from 2a03973 to 3fc73f8 Compare July 7, 2023 08:27
Copy link
Contributor

@nicoprow nicoprow left a comment

Choose a reason for hiding this comment

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

Ok, I thought the failing test execution is a race condition problem but actually the migration versions are duplicated. On the current main there is now already a migration file with version 4_0_0_5, so you would need to increase it. Best is to rebase your branch to main first, then execute the tests again before force pushing

@SujitMBRDI
Copy link
Contributor Author

Ok, I thought the failing test execution is a race condition problem but actually the migration versions are duplicated. On the current main there is now already a migration file with version 4_0_0_5, so you would need to increase it. Best is to rebase your branch to main first, then execute the tests again before force pushing

Done and ready to merge

@SujitMBRDI SujitMBRDI force-pushed the bugfix/gate-persist-legal-form-optional branch from df6c03b to 8cc02e6 Compare July 7, 2023 11:59
@SujitMBRDI SujitMBRDI force-pushed the bugfix/gate-persist-legal-form-optional branch from 8cc02e6 to 5722104 Compare July 7, 2023 12:39
@nicoprow nicoprow merged commit 3752c1b into eclipse-tractusx:main Jul 7, 2023
@nicoprow nicoprow deleted the bugfix/gate-persist-legal-form-optional branch July 7, 2023 12:49
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.

Gate Persistence: Legal Form should be optional
2 participants