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(procedures): implemented gdpr form #13558

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

oalkabouss
Copy link
Contributor

ref:MANAGER-15317

Question Answer
Branch? feat/MANAGER-15315
Bug fix? no
New feature? yes
Breaking change? no
Tickets MANAGER-15317
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Related

@oalkabouss oalkabouss requested review from a team as code owners October 11, 2024 06:00
@oalkabouss oalkabouss requested review from sidlynx, frenautvh, qpavy, sachinrameshn and JacquesLarique and removed request for a team October 11, 2024 06:00
Base automatically changed from feat/MANAGER-15315 to feat/procedures-gdpr October 14, 2024 09:14
@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label Oct 14, 2024
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label Oct 14, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't be modifying this file on this project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to do the test in QA.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should have no impact on the procedures application (as it is not displayed inside the container, and has no dependency to it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to use this in multiple places ?
If not, I'm not sure it is relevant to create a dedicated component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not now. I added it in case it might be useful on another page in the future. Indeed, it adds extra components... I will delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

}
}, [email]);

console.log(isSubmitting);
Copy link
Contributor

Choose a reason for hiding this comment

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

This console log can be cleaned I think

Comment on lines 12 to 16
vi.mock('react-i18next', () => ({
useTranslation: () => ({
t: (key: string) => key,
}),
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary, it is already done in setupTests.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for splitting this from the SelectInput component ?
It seems to unnecessarily split the form logic in multiple parts, unless I'm missing qsomething,
The comment #13558 (comment) also apply here

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

'other_request',
];

export const EmailRegex = /^[a-zA-Z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-zA-Z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z0-9]{2}(?:[a-zA-Z0-9-]*[a-zA-Z0-9])?$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure for that regex ? I never see numbers in the tld part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, i used Xander regex

ref:MANAGER-15317

Signed-off-by: Omar ALKABOUSS MOUSSANA <[email protected]>
ref:MANAGER-15317

Signed-off-by: Omar ALKABOUSS MOUSSANA <[email protected]>
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
12.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@oalkabouss oalkabouss merged commit 6916a19 into feat/procedures-gdpr Oct 17, 2024
12 of 14 checks passed
@oalkabouss oalkabouss deleted the feat/MANAGER-15317 branch October 17, 2024 08:34
oalkabouss added a commit that referenced this pull request Oct 28, 2024
ref:MANAGER-15317

Signed-off-by: Omar ALKABOUSS MOUSSANA <[email protected]>
oalkabouss added a commit that referenced this pull request Nov 26, 2024
ref:MANAGER-15317

Signed-off-by: Omar ALKABOUSS MOUSSANA <[email protected]>
oalkabouss added a commit that referenced this pull request Dec 6, 2024
ref:MANAGER-15317

Signed-off-by: Omar ALKABOUSS MOUSSANA <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants