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.

Sign up for GitHub

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

Jump to bottom

feat(2fa): add redirection rule to pages #11588

Merged
merged 6 commits into from May 17, 2024
Merged

feat(2fa): add redirection rule to pages #11588

merged 6 commits into from May 17, 2024

Conversation

oalkabouss
Copy link
Contributor

@oalkabouss oalkabouss commented Apr 29, 2024 •
edited

ref: MANAGER-14035

Question Answer
Branch? feat/acquisition-storage-2fa
Bug fix? no
New feature? yes
Breaking change? no
Tickets Fix #MANAGER-14035
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 April 29, 2024 16:49
@oalkabouss oalkabouss requested review from kqesar, frenautvh, antonymarion, ghyenne and sachinrameshn and removed request for a team April 29, 2024 16:49
@github-actions github-actions bot added the feature New feature label Apr 29, 2024
oalkabouss added a commit that referenced this pull request Apr 30, 2024
@oalkabouss
feat(2fa): sonar pr feed back (#11588)
8eeb993 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
anooparveti
anooparveti requested changes Apr 30, 2024
View reviewed changes
packages/manager/apps/account-disable-2fa/src/interfaces/index.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please change the folder to types instead of interfaces?
Refer Manager pages for the folder structure.

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 used interfaces because types is ignore in the gitignore file

packages/manager/apps/account-disable-2fa/src/api/index.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Refer "Data fetching and updating in Manager" guide on Manager pages and implement accordingly.

oalkabouss added a commit that referenced this pull request Apr 30, 2024
@oalkabouss
feat(2fa): anoop pr feed back (#11588)
49f32b7 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
JacquesLarique
JacquesLarique reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@JacquesLarique JacquesLarique left a comment

Choose a reason for hiding this comment

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

Can you add some tests for this please.
You may take example on packages/manager/apps/pci-rancher/src/e2e/Error.e2e.ts.

anooparveti
anooparveti reviewed May 3, 2024
View reviewed changes
packages/manager/apps/account-disable-2fa/src/components/Loading/Loading.tsx Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid this Loading component? It does not look like a UX approved behavior to me.

@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label May 7, 2024
JacquesLarique
JacquesLarique reviewed May 10, 2024
View reviewed changes
packages/manager/apps/account-disable-2fa/src/constants/route-path-constants.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

To be compliant with the folder structure definition, this file should be called home.constants.ts and be placed at the same level as the Home page component

oalkabouss added a commit that referenced this pull request May 13, 2024
@oalkabouss
feat(2fa): sonar pr feed back (#11588)
57e9a12 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
oalkabouss added a commit that referenced this pull request May 13, 2024
@oalkabouss
feat(2fa): anoop pr feed back (#11588)
7f89ac4 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label May 13, 2024
anooparveti
anooparveti requested changes May 14, 2024
View reviewed changes
packages/manager/apps/account-disable-2fa/src/pages/home.constants.ts.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove redundant .ts in the file name.

packages/manager/apps/account-disable-2fa/src/pages/home.constants.ts.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the routes constants must be added in the routes folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the routes folder will be at what level? will it also include the routes.ts file?

@oalkabouss
feat(2fa): add redirection rule to pages
316cacb 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@oalkabouss
feat(2fa): sonar pr feed back (#11588)
a5eb1f9 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@oalkabouss
feat(2fa): anoop pr feed back (#11588)
b3d6a72 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@oalkabouss
feat(2fa): add loading to home page
12d2912 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@oalkabouss
feat(2fa): pr feedback add tests
48428cb 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@oalkabouss
feat(2fa): pr feedback
d832c31 
ref: MANAGER-14035

Signed-off-by: oalkabouss <omaralkabous@gmail.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 15, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

@oalkabouss oalkabouss merged commit ef77aac into feat/acquisition-storage-2fa May 17, 2024
13 of 14 checks passed
@oalkabouss oalkabouss deleted the feat/MANAGER-14035 branch May 17, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacquesLarique JacquesLarique JacquesLarique approved these changes

@anooparveti anooparveti anooparveti approved these changes

@kqesar kqesar Awaiting requested review from kqesar kqesar is a code owner automatically assigned from ovh/su-digital-tools-core-stack

@frenautvh frenautvh Awaiting requested review from frenautvh frenautvh is a code owner automatically assigned from ovh/su-digital-tools-core-stack

@antonymarion antonymarion Awaiting requested review from antonymarion antonymarion is a code owner automatically assigned from ovh/team-manager-dev

@ghyenne ghyenne Awaiting requested review from ghyenne ghyenne is a code owner automatically assigned from ovh/team-manager-dev

@sachinrameshn sachinrameshn Awaiting requested review from sachinrameshn sachinrameshn is a code owner automatically assigned from ovh/team-manager-dev

Assignees
No one assigned
Labels
feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oalkabouss @anooparveti @JacquesLarique