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 Issues Dashboard access by URL #207

Merged
merged 12 commits into from
Oct 28, 2023

Conversation

LJXSean
Copy link
Contributor

@LJXSean LJXSean commented Sep 18, 2023

Summary:

Fixes #94

Type of change:

  • ✨ New Feature/ Enhancement

Changes Made:

  • Add CacheRepoFromUrlService that provides getter and setter for repoLocation stored in session storage
  • Add ParseUrlParamsGuard::canActivate that uses CacheRepoFromUrlService to cache the 'org' and 'repo' params from URL into session storage
  • Add path for '/issuesViewer/:org/:repo' to app-routing that uses ParseUrlParamsGuard::canActivate
  • Edit sessionSelectionComponent::autofillRepo to fill and submit repoform if repoLocation is in session storage
  • Subscription to currentAuthState in HeaderComponent now calls openChangeRepoDialog only if user is authenticated and repoLocation not in session storage
  • NOT_AUTHENTICATED_ERROR now thrown for unauthenticated access on protected routes

Screenshots:

With valid repo location
fix-94-success - Trim

With invalid repo location
fix-94-fail - Trim

Proposed Commit Message:

Add path /issuesViewer/:org/:repo

Having functionality to get repo location from url parameters
allows easy distribution of links to a team's repository dashboard.

Let's store the repo location from the new path into session storage
and auto submit the session selection form.

Checklist:

  • I have tested my changes thoroughly.
  • I have created tests for any new code files created in this PR or provided a link to a issue/PR that addresses this.
  • I have added or modified code comments to improve code readability where necessary.
  • I have updated the project's documentation as necessary.

@LJXSean
Copy link
Contributor Author

LJXSean commented Sep 18, 2023

Some implementation details to discuss:

  1. Session Storage persistance after login
  • Possible issue if user logs out and tries to log in to select a different repository, but unable to do so due to session storage persistence that causes session selection form to autofill and submit
  • Maybe reset session storage once form is filled?
  1. Leftover code from CATcher
  • eg. auth::getRepoFromUrlQueryParams and @input(): urlEncodedRepo
  • No such repo query, should such code be removed?
  1. Scope of "Login Required" error
  • Used to give user feedback on why redirect to login when accessing /issuesViewer/:org/:repo
  • Currently implemented in authguard, introduces new behavior for displaying error for all unauthorised access
  • Should this error be shown for all redirects or just that path
  1. Path 'issuesViewer/:org/:repo' always redirects to login even if previously logged in before entering new link
  • Follows current behavior where authentication state not persistant on page refresh
  • eg. WATcher logs out on refresh, not sure if bug or desired behavior
  • Should we make authentication state persistant?
  1. Rename repoUrlCacheService
  • confusing to have 2 caches for repoUrl that serves differnt purposes
  • Should we as rename it as SuggestionService?

@gycgabriel
Copy link
Collaborator

For point 4, this is an issue from CATcher -- CATcher-org/CATcher#709

For point 2 (and perhaps 5 as well), do consider creating another PR/Issue for that

@@ -67,7 +69,7 @@ export class HeaderComponent implements OnInit {
});

this.auth.currentAuthState.subscribe((state) => {
if (auth.isAuthenticated()) {
if (auth.isAuthenticated() && !cacheRepoFromUrlService.hasRepoLocation()) {

Choose a reason for hiding this comment

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

Upon testing, I realised that changing repositories actually leads to having to authenticate again (as you mentioned in your comment). I think it might be good to persist session to sessionStorage in order for better UI/UX. However, I think this works pretty well by itself in the mean time, so let's add it as a separate issue and work on it after this is merged.

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 think gycgabriel mentioned above that this is an existing issue from CATcher.
When we access pages by manually typing in a new url, it refreshes the whole app. Looking into this, I think we could save the github access token to sessionStorage to prevent re-authentication, but I'm not sure of how secure/safe this would be, considering the scope set for the accessToken

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, nice PR and changes look good to go! Have left a couple comments for review, and once those are done, LGTM 🚀

src/app/core/services/cache-repo-from-url.service.ts Outdated Show resolved Hide resolved
src/app/core/services/cache-repo-from-url.service.ts Outdated Show resolved Hide resolved
src/app/core/services/cache-repo-from-url.service.ts Outdated Show resolved Hide resolved
Comment on lines 118 to 123
const repoLocation = this.cacheRepoFromUrlService.repoLocation || this.urlEncodedRepo;
this.repoForm.get('repo').setValue(repoLocation);

if (this.cacheRepoFromUrlService.hasRepoLocation()) {
this.setupSession();
}

Choose a reason for hiding this comment

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

From this, we only set up a session if cacheRepoFromUrlService.repoLocation != null

But setupSession does not use cacheRepoFromUrlService, and it will never set up the session if we are using the url encoded repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi based on my understanding, setupSession does not need cacheRepoFromUrlService since it only uses the value of the repoForm's 'repo' field that will be filled up if hasRepoLocation

Regarding the session not being setup if url encoded repo is used, could you clarify why this is so?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, everything looks good to go! 🚀

@LJXSean
Copy link
Contributor Author

LJXSean commented Oct 27, 2023

Hi @vigneshsankariyer1234567890, if the changes are alright could you help me to merge this PR?
Thank you!

@vigneshsankariyer1234567890 vigneshsankariyer1234567890 merged commit 0650c92 into CATcher-org:main Oct 28, 2023
3 checks passed
@luminousleek luminousleek mentioned this pull request Oct 30, 2023
4 tasks
@damithc
Copy link
Contributor

damithc commented Nov 5, 2023

A follow up note: this kind of features need to be documented in a user guide or else users will never realize they exist.

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.

Add Issues Dashboard access by URL
4 participants