Skip to content
This repository has been archived by the owner on Apr 12, 2023. It is now read-only.

Implementation of AuthGuard #139

Merged
merged 1 commit into from Dec 8, 2018
Merged

Implementation of AuthGuard #139

merged 1 commit into from Dec 8, 2018

Conversation

Khukhunashvili
Copy link
Contributor

@Khukhunashvili Khukhunashvili commented Nov 19, 2018 •
edited

Part of GCI ❤️ 😍

Implementation of AuthGuard, that lets you visit students/organizations route only if user token exists and is not expired.

nikitadhiman
nikitadhiman reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@nikitadhiman nikitadhiman left a comment

Choose a reason for hiding this comment

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

This seems good, but why changes in so many files? Commit only those related to this task otherwise others might face merge issues. Thanks for your awesome work!

nikitadhiman
nikitadhiman reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@nikitadhiman nikitadhiman left a comment

Choose a reason for hiding this comment

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

Won't need login as a part of commit for this task.

@Khukhunashvili
Copy link
Contributor Author

obviously I needed authentication service (auth service is not merged yet) to build AuthGuard, so I built it on top of #107 .
So these file changes include #107 pr & AuthGuard.
In case of merge, It should not throw merge conflicts. (maybe only canActivate: [AuthGuard] which is easy to resolve )

@nikitadhiman
Copy link
Contributor

While we consider this for merge, might ask you to make some changes. Thanks

@nikitadhiman
Copy link
Contributor

This is good considering task implementation point of view. Submit your task for approval

@nikitadhiman
Copy link
Contributor

@khukhuna , please resolve conflicts. This is being considered for merge😅😀

aashutoshrathi
aashutoshrathi suggested changes Dec 7, 2018
View reviewed changes
Copy link

@aashutoshrathi aashutoshrathi left a comment

Choose a reason for hiding this comment

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

Check comments

src/environments/environment.ts Outdated Show resolved Hide resolved
src/app/app.component.html Outdated Show resolved Hide resolved
@aashutoshrathi
Copy link

Also, resolve conflicts and add a GIF before we test it. Thanks

@nikitadhiman nikitadhiman merged commit 1d81f95 into JBossOutreachArchive:master Dec 8, 2018
@aashutoshrathi
Copy link

Thanks 💯

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nikitadhiman nikitadhiman nikitadhiman left review comments

@aashutoshrathi aashutoshrathi aashutoshrathi requested changes

Assignees
No one assigned
Labels
Good For Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Khukhunashvili @nikitadhiman @aashutoshrathi