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

feat(certificate): db enablement company certificates #475

Conversation

AnuragNagpure
Copy link
Contributor

Description

Enable companies to fetch their company certificates with the respective document status.

Why

To ensure that the feature can get supported, first changes on db level are needed ++ seeding data adjustments. Tables added are mentioned below
CompanyCertificate
CompanyCertificateStatus
CompanyCertificateType
CompanyCertificateTypeDescription

Issue

#471

Checklist

Please delete options that are not relevant.

  • I have followed the contributing guidelines
  • I have performed a self-review of my own code
  • I have successfully tested my changes locally
  • I have added tests that prove my changes work
  • I have checked that new and existing tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas

@AnuragNagpure AnuragNagpure requested review from Phil91 and ntruchsess and removed request for ntruchsess and Phil91 February 1, 2024 09:00
Copy link
Member

@Phil91 Phil91 left a comment

Choose a reason for hiding this comment

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

@jjeroch the implementation in this PR adds a Id field to the company_certificates table. That differentiate to the description in the issue and I couldn't find a discussion in the issue nor Jira. Main reason for that is if we don't have the field a company could only have one entry linked to company_certificate_type. Example BMW has an ISO 9001certificate which is inactive, they could never create a new one, only way to have an ISO 9001 certificate is to update the inactive one, in my opinion we shouldn't do that, do you agree with that or should Anurag remove the Id field?

@AnuragNagpure one general thing I recognised, please use conventional commits not only for the PR title. Please use it for your commit messages as well

@jjeroch
Copy link
Contributor

jjeroch commented Feb 2, 2024

@jjeroch the implementation in this PR adds a Id field to the company_certificates table. That differentiate to the description in the issue and I couldn't find a discussion in the issue nor Jira. Main reason for that is if we don't have the field a company could only have one entry linked to company_certificate_type. Example BMW has an ISO 9001certificate which is inactive, they could never create a new one, only way to have an ISO 9001 certificate is to update the inactive one, in my opinion we shouldn't do that, do you agree with that or should Anurag remove the Id field?

@AnuragNagpure one general thing I recognised, please use conventional commits not only for the PR title. Please use it for your commit messages as well

Hi @Phil91 & @AnuragNagpure fully agree with your proposal. BMW must have the chance to have an ISO 9001certificate in status "INACTIVE" while a second is in status "ACTIVE"

@AnuragNagpure AnuragNagpure requested a review from Phil91 February 2, 2024 13:22
@jjeroch jjeroch added the priority PR needs to prioritized at review label Feb 4, 2024
@jjeroch jjeroch added this to the Version CX Release 24.03 milestone Feb 4, 2024
@ntruchsess ntruchsess force-pushed the feature/CPLP-3642-db-enablement-company-certificates branch 2 times, most recently from b25a7ad to 9a0f301 Compare February 6, 2024 00:44
@ntruchsess ntruchsess force-pushed the feature/CPLP-3642-db-enablement-company-certificates branch from 9a0f301 to 584ed39 Compare February 6, 2024 00:56
Copy link

sonarqubecloud bot commented Feb 6, 2024

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

@ntruchsess
Copy link
Contributor

to end up with a single 1.8.0-rc5-migration the changes from #389 have been merged and the migration has been recreated.

@ntruchsess ntruchsess merged commit d608a73 into eclipse-tractusx:release/v1.8.0-RC5 Feb 6, 2024
6 checks passed
@ntruchsess ntruchsess deleted the feature/CPLP-3642-db-enablement-company-certificates branch February 6, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority PR needs to prioritized at review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants