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

Login to Github before entering repository name #169

Merged
merged 26 commits into from
Sep 10, 2023

Conversation

chia-yh
Copy link
Contributor

@chia-yh chia-yh commented Jul 12, 2023

Summary:

Attempt to resolve #154

WATcher allows users to change repositories, i.e., the repository is not fixed like in CATcher. Having users login first, then being able to choose the repository they wish to see would allow for a smoother UX, e.g.

Login -> Sign in to Github -> Confirm Login Account -> Enter Repostory URL -> Issues Dashboard

Currently displays a minimal login card that redirects users to sign in to Github. Future enhancements to the UI would be desirable.

Type of change:

  • ✨ New Feature/ Enhancement

Changes Made:

  • Add login.component, which extracts the AuthService::startOAuthProcess() call from session-selection.component.ts that allows users to login to Github
  • Add some tests for login.component in login.component.spec.test
  • Extract user data setup from ConfirmLoginComponent::completeLoginProcess() to AuthService::setRepo(), leaving the creation of the user model, after which authentication state is changed to authenticated
  • Move handleAuthSuccess() from ConfirmLoginComponent to AuthService, renamed to handleSetRepoSuccess()
  • Modify ConfirmLoginComponent tests as necessary
  • Add repoSetSource, repoSetState, isRepoSet() to PhaseService to determine whether app-session-selection is displayed
  • Change auth.component.html to show LoginComponent instead of SessionSelectionComponent by default
  • Change auth.component.html to prevent current session from being shown on the login card for confirming login account, as no session has been set at that point of the process
  • initializeRepoNameInTitle() method call in HeaderComponent now subscribes to repoSetState instead of currentAuthState, and checks isAuthenticated() && isRepoSet() before calling the method, html uses isRepoSet() to display buttons requiring a current repository after one has been set

Screenshots:

login_before_select_session

Proposed Commit Message:

Change auth process to login before selecting repo

Users select a repository before login.

WATcher does not fix a single repository, and allows users to change the
selected repository.

Having users login to Github prior to selecting a repository would
facilitate a smoother UX.

Let's allow users to login before choosing a repository.

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.

Copy link
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

Tested, works good 👍
Will check code next.

src/app/auth/confirm-login/confirm-login.component.ts Outdated Show resolved Hide resolved
tests/app/auth/login/login.component.spec.ts Outdated Show resolved Hide resolved
src/app/auth/login/login.component.ts Show resolved Hide resolved
tests/app/auth/login/login.component.spec.ts Show resolved Hide resolved
src/app/auth/auth.component.html Outdated Show resolved Hide resolved
gycgabriel
gycgabriel previously approved these changes Jul 17, 2023
Copy link
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

LGTM

@gycgabriel gycgabriel requested a review from a team July 17, 2023 03:04
@gycgabriel
Copy link
Collaborator

Let's have at least two reviews before merging this PR.

@chia-yh
Copy link
Contributor Author

chia-yh commented Jul 17, 2023

While working on #154 (comment) , it occurred to me that I could make some changes to the current PR that would result in what I believe to be an improvement to the current implementation. I've made the changes in this branch:

  • AuthComponent: removed SessionSetupStateSubscription and associated variables/methods, replaced with isRepoSet(), which instead simply checks whether a repo has been set, which determines whether app-session-selection is displayed by AuthComponent
  • AuthService: replaced sessionSetupStateSource, sessionSetupState, etc. with repoSetSource, repoSetState, isRepoSet()
  • HeaderComponent: initializeRepoNameInTitle() method call now subscribes to repoSetState instead of currentAuthState, and checks isAuthenticated() && isRepoSet() before calling the method, html uses isRepoSet() to display buttons requiring a current repository after one has been set
  • Modified ConfirmLoginComponent tests as necessary

These changes remove the code for setting up a separate subscription in AuthComponent, and instead checks if a repo has been set to determine when SessionSelectionComponent should be brought up. The initialization of the repository name in the title is also now tied to an observable that represents whether a repository has been set instead of the state of authentication. This allows the state to be set to AuthState.Authenticated right after the user has been authenticated, instead of after the user has been authenticated and the repository has been set, which I feel represents the current state of WATcher more accurately. In terms of visible changes to the user, as the visual changes in HeaderComponent is tied to isAuthenticated(), the session selection screen after the user is confirmed now looks like this:
image

instead of like this (current state of the PR):
image

The user is able to logout at this point, but is unable to jump to a repository or sync with Github as there is no repository set, and "(Issue Dashboard)" is displayed, showing the current phase WATcher is in.

If the changes in the branch seem appropriate, I can merge the commits from the branch onto this PR's branch.

@chia-yh chia-yh changed the title Login before select session Login to Github before entering repository name Jul 17, 2023
Copy link
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

We are decoupling setting repository from auth, so it is better to move setting repository stuff to phase.service.ts

src/app/core/services/auth.service.ts Outdated Show resolved Hide resolved
src/app/core/services/auth.service.ts Outdated Show resolved Hide resolved
src/app/core/services/auth.service.ts Outdated Show resolved Hide resolved
src/app/auth/auth.component.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

The select repo component is not hidden in the page.

Scrolling down on the logging page will reveal the component

src/app/auth/auth.component.html Outdated Show resolved Hide resolved
src/app/auth/auth.component.ts Show resolved Hide resolved
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

Right now, invalid repository like (" ") will always bring the user to the issueViewer phase.
This will then lead to a lot of error prompts.

image
(Empty repo name)


There seems to be 'other forces' at work here as the application is still navigating to issueViewer before handleSetRepoSuccess() is not called. (not sure why this is the case)

src/app/core/services/auth.service.ts Outdated Show resolved Hide resolved
src/app/core/services/auth.service.ts Outdated Show resolved Hide resolved
private logger: LoggingService
) {}

ngOnInit() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ngOnInit() {}

ngOnInit() is not used. Let's remove implements OnInit!

Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

LGTM!

@chia-yh
Copy link
Contributor Author

chia-yh commented Aug 12, 2023

@gycgabriel could I check if there are any further comments with respect to changes that need to be made to this PR?

gycgabriel
gycgabriel previously approved these changes Aug 12, 2023
Copy link
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

LGTM, my comments have been addressed.

gycgabriel
gycgabriel previously approved these changes Aug 17, 2023
Copy link
Collaborator

@gycgabriel gycgabriel 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
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

LGTM

@gycgabriel gycgabriel merged commit 237a997 into CATcher-org:main Sep 10, 2023
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.

Login to Github before entering repository name
3 participants