[RHOAIENG-1154]: Apply modal to alert user error #3512

jenny-s51 · 2024-11-22T16:40:48Z

Towards https://issues.redhat.com/browse/RHOAIENG-1154.

Description

Before:

After:

How Has This Been Tested?

Run application.cy.ts to simulate 403 status code and verify the modal is rendered.

Test Impact

Added new test to application.cy.ts.

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

emilys314 · 2024-11-26T18:32:37Z

Would there be a way to detect specifically that the user's session timed out to show the new modal? I feel like it may be slightly misleading because it looks like the code is a catch all, but the message is specific for session expiration? Unless that was the idea, it also doesn't seem that straightfoward to figure out a expired session

christianvogt · 2024-11-26T19:40:33Z

I agree, we should look to identify the exact error response that comes from an expired session to show this modal otherwise we show the old error or update it with some other text if it's not related to expired session.

xianli123 · 2024-11-27T01:49:37Z

Thanks @jenny-s51. The implementation looks good to me.
As for the description in the modal, it's based on the context of RHOAIENG-1154. I agree with @emilys314 @christianvogt. It would be better to identify the exact errors, and then we can update the description in the modal. If you have any concrete idea, please let me know and I can provide new design if needed.

openshift-ci · 2024-12-18T22:12:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jenny-s51

Thank you for your feedback here @emilys314 @christianvogt @xianli123 - after investigating with @christianvogt we identified that a 403 status code indicates an expired session.

I've updated the implementation to render the modal when that status code is received, and added a corresponding Cypress test.

christianvogt · 2025-01-06T21:15:32Z

frontend/src/__tests__/cypress/cypress/pages/loginDialog.ts

+export class LoginDialog {
+  findText(): Chainable<JQuery<HTMLElement>> {
+    return cy.findByTestId('timeout-text');
+  }
+}


For modals we normally create a contextual page object which extends the Modal class:

Suggested change

export class LoginDialog {

findText(): Chainable<JQuery<HTMLElement>> {

return cy.findByTestId('timeout-text');

}

}

class LoginDialog extends Modal {

constructor() {

super('Session Expired');

}

}

With a new instance of this class you can then do loginDialog().shouldBeOpen() instead of the modal text assertion.

Applied this change with loginDialog.shouldBeOpen() - the test isn't passing though because the find() call in shouldBeOpen doesn't identify this modal correctly. Investigating...

christianvogt · 2025-01-06T21:17:17Z

frontend/src/app/App.tsx

+        <Modal variant={ModalVariant.small} isOpen ouiaId="BasicModal">
+          <ModalHeader title="Session Expired" titleIconVariant="warning" />
+          <ModalBody data-testid="timeout-text">
+            Your session timed out. To continue working, log in.
+          </ModalBody>
+          <ModalFooter>
+            <Button
+              key="confirm"
+              variant="primary"
+              onClick={() => logout().then(() => window.location.reload())}
+            >
+              Log in
+            </Button>
+          </ModalFooter>
+        </Modal>


Can you move this Modal to a separate file for better separation.

Yes, thanks, created a separate component for this.

christianvogt · 2025-01-06T21:18:18Z

frontend/src/__tests__/cypress/cypress/tests/mocked/application.cy.ts

@@ -113,4 +114,22 @@ describe('Application', () => {
    aboutDialog.findText().should('contain.text', 'OpenShift');
    aboutDialog.findProductName().should('contain.text', 'OpenShift AI');
  });
+  it('should show the login modal when receiving a 403 status code', () => {


This test can also intercept the call to /oauth/sign_out when the user clicks the Log in button.

frontend/src/services/dashboardConfigService.ts

christianvogt · 2025-01-08T16:51:24Z

frontend/src/__tests__/cypress/cypress/pages/loginDialog.ts

+class LoginDialog extends Modal {
+  protected testId;
+
+  constructor(title: string, testId: string) {
+    super(title);
+    this.testId = testId;
+  }
+
+  find() {
+    return cy.findByTestId(this.testId);
+  }
+
+  findLoginButton() {
+    return this.findFooter().findByTestId('modal-login-button');
+  }
+}
+
+export const loginDialog = new LoginDialog('Session Expired', 'session-expired-modal');


No need to parameterize the constructor since you're not going to instantiate different login dialogs:

Suggested change

class LoginDialog extends Modal {

protected testId;

constructor(title: string, testId: string) {

super(title);

this.testId = testId;

}

find() {

return cy.findByTestId(this.testId);

}

findLoginButton() {

return this.findFooter().findByTestId('modal-login-button');

}

}

export const loginDialog = new LoginDialog('Session Expired', 'session-expired-modal');

class LoginDialog extends Modal {

constructor() {

super('Session Expired');

}

find() {

return cy.findByTestId('session-expired-modal');

}

findLoginButton() {

return this.findFooter().findByTestId('modal-login-button');

}

}

export const loginDialog = new LoginDialog();

christianvogt · 2025-01-08T16:53:32Z

frontend/src/__tests__/cypress/cypress/tests/mocked/application.cy.ts

+    }).as('getData403');
+
+    // Set up the sign-out intercept before visiting the page
+    cy.intercept('GET', '/oauth/sign_out').as('signOut');


It would be good to add a typed interface function to the interceptOdh method. We tend to do this for all our well known URLs.

frontend/src/app/App.tsx

frontend/src/app/SessionExpiredModal.tsx

frontend/src/services/dashboardConfigService.ts

check for 403 status code add cypress test update test title format format format remove it.only fix relative import PR feedback from Christian, WIP Cypress throwing error on shouldBeOpen fix tests, fix linting issues revert find() function revert find, update comment format apply PR comments from review format remove response param uncomment simulate login button remove it.only reorder

codecov · 2025-01-08T22:36:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.04%. Comparing base (9a2449c) to head (ab7ef3f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3512      +/-   ##
==========================================
+ Coverage   85.01%   85.04%   +0.02%     
==========================================
  Files        1404     1405       +1     
  Lines       32244    32247       +3     
  Branches     9042     9044       +2     
==========================================
+ Hits        27412    27424      +12     
+ Misses       4832     4823       -9

Files with missing lines	Coverage Δ
frontend/src/app/App.tsx	`90.90% <100.00%> (+0.90%)`	⬆️
frontend/src/app/SessionExpiredModal.tsx	`100.00% <100.00%> (ø)`
frontend/src/app/useApplicationSettings.tsx	`79.31% <100.00%> (+10.34%)`	⬆️
frontend/src/services/dashboardConfigService.ts	`100.00% <100.00%> (+33.33%)`	⬆️

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a2449c...ab7ef3f. Read the comment docs.

openshift-ci bot requested review from christianvogt and dpanshug November 22, 2024 16:40

jenny-s51 removed the request for review from dpanshug November 22, 2024 16:41

jenny-s51 marked this pull request as draft November 22, 2024 16:41

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Nov 22, 2024

jenny-s51 force-pushed the RHOAIENG-1154 branch from 2dc38bb to d33ca1c Compare November 26, 2024 15:00

jenny-s51 marked this pull request as ready for review November 26, 2024 15:10

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Nov 26, 2024

openshift-ci bot requested review from DaoDaoNoCode and pnaik1 November 26, 2024 15:10

jenny-s51 force-pushed the RHOAIENG-1154 branch from d33ca1c to cd37c9c Compare December 18, 2024 22:12

jenny-s51 force-pushed the RHOAIENG-1154 branch from 39e8c1e to 3c6f22e Compare December 19, 2024 22:50

jenny-s51 commented Dec 19, 2024

View reviewed changes

jenny-s51 force-pushed the RHOAIENG-1154 branch 3 times, most recently from d26db87 to 07e11ba Compare December 20, 2024 14:21

christianvogt reviewed Jan 6, 2025

View reviewed changes

jenny-s51 force-pushed the RHOAIENG-1154 branch 2 times, most recently from 3339be0 to 020337d Compare January 8, 2025 16:51

christianvogt reviewed Jan 8, 2025

View reviewed changes

jenny-s51 force-pushed the RHOAIENG-1154 branch from f61696f to 23fb1b1 Compare January 8, 2025 21:52

jenny-s51 force-pushed the RHOAIENG-1154 branch from 23fb1b1 to ab7ef3f Compare January 8, 2025 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHOAIENG-1154]: Apply modal to alert user error #3512

[RHOAIENG-1154]: Apply modal to alert user error #3512

jenny-s51 commented Nov 22, 2024 •

edited

Loading

emilys314 commented Nov 26, 2024 •

edited

Loading

christianvogt commented Nov 26, 2024

xianli123 commented Nov 27, 2024

openshift-ci bot commented Dec 18, 2024

jenny-s51 left a comment

christianvogt Jan 6, 2025

jenny-s51 Jan 7, 2025

christianvogt Jan 6, 2025

jenny-s51 Jan 7, 2025

christianvogt Jan 6, 2025

christianvogt Jan 8, 2025

christianvogt Jan 8, 2025

codecov bot commented Jan 8, 2025

[RHOAIENG-1154]: Apply modal to alert user error #3512

Are you sure you want to change the base?

[RHOAIENG-1154]: Apply modal to alert user error #3512

Conversation

jenny-s51 commented Nov 22, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

emilys314 commented Nov 26, 2024 • edited Loading

christianvogt commented Nov 26, 2024

xianli123 commented Nov 27, 2024

openshift-ci bot commented Dec 18, 2024

jenny-s51 left a comment

Choose a reason for hiding this comment

christianvogt Jan 6, 2025

Choose a reason for hiding this comment

jenny-s51 Jan 7, 2025

Choose a reason for hiding this comment

christianvogt Jan 6, 2025

Choose a reason for hiding this comment

jenny-s51 Jan 7, 2025

Choose a reason for hiding this comment

christianvogt Jan 6, 2025

Choose a reason for hiding this comment

christianvogt Jan 8, 2025

Choose a reason for hiding this comment

christianvogt Jan 8, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2025

Codecov Report

jenny-s51 commented Nov 22, 2024 •

edited

Loading

emilys314 commented Nov 26, 2024 •

edited

Loading