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

Add login redirect #1256

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Conversation

nknguyenhc
Copy link
Contributor

Summary:

Fixes #1229

Changes Made:

  • Allow the application to redirect to next route if any.
  • Checks if the next route is allowed before redirecting to it.
  • Stores the next route in session storage.

Unlike CATcher-org/WATcher#255,

  • Routes are already changed when each view changes. This PR simply aims to redirect to the target route after logging in.
  • This is meant to work with any phase.

Note that it only redirects to the current phase. If there are multiple open phases, the app only allows redirection to the "current phase", which is the first available phase. However, given that in CATcher, only one phase is allowed at a time,

Proposed Commit Message:

Add login redirect

Previously, upon login, users are brought to the
default landing page of the phase.

We redirect to the intended landing page,
with appropriate next route checking.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 54.62%. Comparing base (5e7ed48) to head (951e632).

Files Patch % Lines
src/app/core/services/auth.service.ts 9.09% 9 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
- Coverage   54.82%   54.62%   -0.21%     
==========================================
  Files         105      105              
  Lines        2858     2867       +9     
  Branches      503      504       +1     
==========================================
- Hits         1567     1566       -1     
- Misses       1030     1038       +8     
- Partials      261      263       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Arif-Khalid Arif-Khalid left a comment

Choose a reason for hiding this comment

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

Clean and effective method of storing and using the original URL. Let's wait for senior review before merging but LGTM!

@luminousleek
Copy link
Contributor

Using an invalid URL (e.g. linking to an issue that doesn't exist) should redirect to the default landing page, but currently displays a blank page.
Screenshot 2024-04-01 at 1 10 43 PM

@nknguyenhc
Copy link
Contributor Author

Using an invalid URL (e.g. linking to an issue that doesn't exist) should redirect to the default landing page, but currently displays a blank page. Screenshot 2024-04-01 at 1 10 43 PM

Fixed! In ViewIssueComponent::getAndPollIssue, I redirect to current phase landing page if there is error polling the issue.

@nknguyenhc nknguyenhc requested a review from luminousleek April 8, 2024 05:13
@luminousleek
Copy link
Contributor

Fixed! In ViewIssueComponent::getAndPollIssue, I redirect to current phase landing page if there is error polling the issue.

Cool, is it possible to have an error popup when this happens?

Copy link
Contributor

@cheehongw cheehongw left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@luminousleek luminousleek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@nknguyenhc nknguyenhc merged commit 9690676 into CATcher-org:master Apr 15, 2024
4 of 5 checks passed
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.

Link redirects to login page, but after logging in, not brought to the intended landing page
5 participants