Add shareable repo-specific URL #255

nknguyenhc · 2024-02-14T15:54:34Z

Summary:

Fixes #251

Type of change:

✨ New Feature/ Enhancement

Changes Made:

For every change in repo, the URL query param changes. For example, if you are switching from another repo to this repo, the URL changes to /issuesViewer?repo=CATcher-org%2FWATcher.
When user keys a URL with pathname (i.e. needs to login before proceeding), the pathname + query params are stored in session storage.
In authentication page, if there is a URL to be redirected to (in session storage), it automates the login process.
Appropriate error handling is added so that wrong URL input are handled properly.

I did try to limit the changes in this PR to as small as possible, but I could not find a good way to split this task to smaller task, such that smaller tasks can be independently tested.

Some of the sample URLs I have tried:

/issuesViewer: invalid URL, because now issue viewer URL must have query param
/issuesViewer?repo=CATcher-org%2FWATcherrr: invalid URL, because repo does not exist
/issuesViewer?repo=CATcher-org%2FWATcher%2Ftest: invalid URL, because repo name is of wrong format
/issuesViewer?repo=CATcher-org%2FWATcher: valid URL
Invalid URL, followed by pathname /: login should proceed as per normal
Valid URL, followed by pathname /: login should proceed as per normal

Future enhancements:

Here are some possible future enhancements of this feature:

Store filters, including labels, milestones, etc, in query params to allow filters to be shareable through links
Redirect to activity dashboard if URL is indicated as such, when the activity dashboard is available

Screenshots:

NIL

Proposed Commit Message:

Add shareable repo-specific URL

Previously, URLs are fixed upon repo change, hence URLs are not shareable.

Let's make the current repo visible in the URL, so that it shareable.

Furthermore, login is automated when there is such next route to be redirected to.

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.

luminousleek · 2024-02-17T13:03:41Z

Thanks for this PR! A quick test does show that the username doesn't display in the top right corner after logging in though, see the screenshot below.

luminousleek · 2024-02-17T13:10:10Z

src/app/core/services/auth.service.ts

@@ -33,6 +33,9 @@ export enum AuthState {
 * updating the application state with regards to authentication.
 */
 export class AuthService {
+  private static readonly DEFAULT_HAS_PRIVATE_PERMISSION = true;


Not sure if this should be the default. See #214.

I have changed this to false. This means that auto-redirect does not work if the user paste the link to view a private repo. There needs extra work to handle viewing private repos, I will create a separate issue.

nknguyenhc · 2024-02-18T05:53:38Z

I have fixed the bug! Let me know if there are any other issues.

luminousleek

Looks almost good to merge! Just a really small nit to fix.

luminousleek · 2024-02-19T14:23:20Z

src/app/core/services/auth.service.ts

@@ -33,6 +33,9 @@ export enum AuthState {
 * updating the application state with regards to authentication.
 */
 export class AuthService {
+  private static readonly DEFAULT_HAS_PRIVATE_PERMISSION = false;


Small nit - might want to change the constant name here

I changed the constant name to DEFAULT_HAS_PERMISSION_TO_PRIVATE_REPOS

hmm what i meant is that the constant name sort of implies that the constant value is true, but it's actually false. maybe something like DEFAULT_DOES_NOT_HAVE_PRIVATE_REPO_PERMISSION might work

To avoid the name being interpreted as a boolean question, I have renamed it to NO_PERMISSION_TO_PRIVATE_REPOS

In my opinion I get where @luminousleek is coming from. Default should indicate its default state in the name following the word DEFAULT.
However, this is confusing since developers might mistake it as representing a boolean variable which means "No private repo access", when really it is used as "has private repo access".
I think a better name would be just HAS_PERMISSION_TO_PRIVATE_REPOS. why the DEFAULT at all when it is implied already by static readonly.

Without the word DEFAULT, it might be mistaken that the auth service will never have access to private repos, while in fact, this is only the default settings when the app skips manual login. The app would still have access to private repos if the user chooses to (with manual login).

Phrasing the name as a boolean question (i.e. HAS_PERMISSION_TO_PRIVATE_REPOS) is more applicable when it is a boolean variable that can take up both values (i.e. true and false) in different scenarios, but in this case, it can only be false. Some might infer the variable name as the auth service always have access to private repos.

I would still be more inclined to use NO_PERMISSION_TO_PRIVATE_REPOS, because it really means "no private repo access", and is meant to be used only when the app has no private repo access. Unless there is a good way to incorporate the word DEFAULT into the constant name when phrasing it as a boolean question.

Late to the discussion, I would prefer DEFAULT_NO_PERMISSION_TO_PRIVATE_REPOS or similar. Default keyword is important to show that it is by default

Changed to DEFAULT_NO_PERMISSION_TO_PRIVATE_REPOS!

Arif-Khalid · 2024-02-20T14:21:05Z

Tested all functionality and working as intended. Aside from naming nitpick by @luminousleek, LGTM!

gycgabriel

Late to the discussion, I would prefer DEFAULT_NO_PERMISSION_TO_PRIVATE_REPOS or similar. Default keyword is important to show that it is by default

luminousleek

LGTM, this will be a useful feature! Thanks for your hard work.

nknguyenhc added 6 commits February 14, 2024 17:09

Automate authentication if has next

f4ff281

Reorder static constants in AuthService

036adb8

Parsing and redirecting to next route

d216d1f

Update import order

070d83a

Merge branch 'main' into url-indicate-repo

712b3d9

Clear next on repo not found

58f2f6b

luminousleek reviewed Feb 17, 2024

View reviewed changes

Fix username not seen & default private access

82305c9

Remove unnecessary changes

fcb47e0

luminousleek requested changes Feb 19, 2024

View reviewed changes

nknguyenhc added 2 commits February 20, 2024 21:05

Update constant name

1f83aae

Update constant name

93c1a1f

gycgabriel reviewed Feb 22, 2024

View reviewed changes

Update constant name

80f5859

nknguyenhc requested a review from luminousleek February 24, 2024 08:43

luminousleek approved these changes Feb 24, 2024

View reviewed changes

nknguyenhc merged commit 3db1c8a into CATcher-org:main Feb 26, 2024
3 checks passed

NereusWB922 mentioned this pull request Mar 3, 2024

Update test cases for phase service #275

Merged

This was referenced Mar 4, 2024

Link redirects to login page, but after logging in, not brought to the intended landing page CATcher-org/CATcher#1229

Closed

Add current filters to URL #277

Closed

Make URL redirect to work with activity dashboard #278

Open

nknguyenhc mentioned this pull request Mar 12, 2024

Add login redirect CATcher-org/CATcher#1256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shareable repo-specific URL #255

Add shareable repo-specific URL #255

nknguyenhc commented Feb 14, 2024 •

edited

Loading

luminousleek commented Feb 17, 2024

luminousleek Feb 17, 2024

nknguyenhc Feb 18, 2024

nknguyenhc commented Feb 18, 2024

luminousleek left a comment

luminousleek Feb 19, 2024

nknguyenhc Feb 20, 2024

luminousleek Feb 20, 2024

nknguyenhc Feb 20, 2024

Arif-Khalid Feb 20, 2024

nknguyenhc Feb 21, 2024

gycgabriel Feb 22, 2024

nknguyenhc Feb 24, 2024

Arif-Khalid commented Feb 20, 2024

gycgabriel left a comment

luminousleek left a comment

Add shareable repo-specific URL #255

Add shareable repo-specific URL #255

Conversation

nknguyenhc commented Feb 14, 2024 • edited Loading

Summary:

Type of change:

Changes Made:

Future enhancements:

Screenshots:

Proposed Commit Message:

Checklist:

luminousleek commented Feb 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknguyenhc commented Feb 18, 2024

luminousleek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arif-Khalid commented Feb 20, 2024

gycgabriel left a comment

Choose a reason for hiding this comment

luminousleek left a comment

Choose a reason for hiding this comment

nknguyenhc commented Feb 14, 2024 •

edited

Loading