Feat: Issue#30 protected page and Persistent Token #34

mtreacy002 · 2020-07-01T11:15:51Z

Description

Add ProtectedPage functionality to separate Public vs Private/Members-only pages

Fixes #30

Type of Change:

Code
User Interface

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

by running NPM test to make sure all existing tests still pass
testing manually by runnning the app and confirm the followings:
- on initial landing page, the navbar shows Register and Login options
- trying to access 'My Space' page (which is a private member page) when user not login, resulted in redirection to the login page, which is the expected behaviour.
- after successful login, user is redirected to homepage and navbar is showing Logout option
- while user logged-in, 'My Space' page is accessible directly (without redirection to login page)
- after user logout, user gets redirected to the homepage and the navbar tab back to showing Register and Login options. 'My Space' page also no longer accessible.

Updated gif link

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials

Code/Quality Assurance Only

My changes generate no new warnings

mtreacy002 · 2020-07-02T13:47:59Z

Updatte @anitab-org/bridgeintech-maintainers . I've just pushed the latest code. Please give your feedback. In the meantime I'll start backend GET /user profile.

ramitsawhney27 · 2020-07-02T13:56:40Z

src/login/Login.jsx

        fetch("http://127.0.0.1:5000/login", requestLogin)
            .then(async response => {
                let data = await response.json();
                if (response.status === 200) {
+                    if (data !== {}) {


Is your purpose to see if data is an empty object?
Please look into considerations for such tests, and I would recommend using one of the suggestions from here: https://stackoverflow.com/questions/679915/how-do-i-test-for-an-empty-javascript-object

Another resource: https://stackoverflow.com/questions/4994201/is-object-empty

Thanks, @ramitsawhney27 . I've changed it to one of the suggested option (using underscore library _.isEmpty(obj)) 👍

src/login/Login.jsx

ramitsawhney27 · 2020-07-02T13:57:16Z

src/login/Login.jsx

@@ -25,21 +25,26 @@ export default function Login() {
            },
            body: JSON.stringify(payload)
        };
-
        fetch("http://127.0.0.1:5000/login", requestLogin)


Should be a constant, like LOCALHOST="http://...."

done. Changed it to BASE_API_URL

mtreacy002 · 2020-07-02T15:27:05Z

Update @anitab-org/bridgeintech-maintainers and @ramitsawhney27 . I have made the changes as per @ramitsawhney27 suggestion. Please let me know if you have any other feedback 😉.

ramitsawhney27

The changes look good to me, great work on the quick turnaround @mtreacy002!

Stale

mtreacy002 · 2020-07-03T09:51:30Z

Just wanna share latest codesandbox incase anyone wanna try it out.
https://codesandbox.io/s/persistence-js-cookies-u0007?file=/src/Routes.js

Note: for some reason the app doesn't work the same on sandbox and VSCode. not sure why. To test the final code change, try to test directly from PR branch.

mtreacy002 · 2020-07-03T12:20:29Z

Update @meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . I've fixed the bugs. Now this PR is complete. Logout option on Navbar now automatically shown after successful login.

src/Navigation.js

meenakshi-dhanani · 2020-07-03T17:21:09Z

src/Routes.js

-);
+export const SessionUser = createContext(Cookies.get("user"));
+export const BASE_API_URL = "http://127.0.0.1:5000";
+


We could abstract all this Context to a separate component

I finally found a good example:
https://codesandbox.io/s/p71pr7jn50?file=/src/AuthContext.js

You can explain more on this in our catch up today.

meenakshi-dhanani · 2020-07-03T17:28:10Z

Maya this is some great work! Especially because I know you had to research about everything and I haven't been of much help lately. Great work!
There is a slightly cleaner way of abstracting out the AuthContext, I found this just today, apologies for sharing it so late - https://codesandbox.io/s/p71pr7jn50?

We can pair and work on the changes together during our catchup.

…NavLink href as to

Protected page fix

mtreacy002 · 2020-07-08T00:37:52Z

Update @meenakshi-dhanani . I've merged your changes into this PR branch. Thanks for the fix. It looks great 😉.

meenakshi-dhanani · 2020-07-08T17:50:12Z

There is an unnecessary component for Logout, it could be done within a method and history.push instead of Redirect. It's fine though if we tackle that improvement in a separate PR. Since I understand this PR has been open for long.

meenakshi-dhanani

I have tested the following scenarios:

User has to login to access MySpace page
Once user logs in they can access MySpace
On logout, MySpace again redirects to Login
Other public pages can be viewed without Login

Also performed code review for the PR.

meenakshi-dhanani · 2020-07-08T17:53:57Z

@anitab-org/bridgeintech-maintainers can you review this PR?

meenakshi-dhanani · 2020-07-09T11:29:01Z

@foongminwong can you review this PR?

foongminwong · 2020-07-09T17:23:32Z

@foongminwong can you review this PR?

Yes I'll review this by today

foongminwong

The changes made in this PR were tested locally. Following are the results:

Code review - Done by @meenakshi-dhanani and @ramitsawhney27
All possible responses were tested as below:

Protected page & Persistent Token UI	Expected Result	Actual Result
	As a user, when I click on `My Space`, it prompts me to login.	✅
	As a user, after I login into `My Space`, I should see a BIT private page for member's portfolio under `My Space`. A welcome message should show up and indicate that the user logs in successfully.	✅
	As a user, when I logout from `My Space`, it should direct me to a BIT public landing page.	✅

Additional testcases covered: N/A
OS Version: Windows 10

mtreacy002 added 5 commits July 1, 2020 17:31

issue#30 Protected page and persistence token

777ebbe

remove if else from Router

1fb71df

separate Navbar for toggle login/logout

4b8dfc2

Change default successful login to reroute to Home

604b677

Remove logged in status on Register page

966833b

mtreacy002 self-assigned this Jul 1, 2020

mtreacy002 added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Category: User Interface Improvements or additions to design. Program: GSOC Related to work completed during the Google Summer of Code Program. labels Jul 1, 2020

mtreacy002 modified the milestone: GSoC: Coding Phase I Jul 1, 2020

mtreacy002 added 2 commits July 2, 2020 23:35

PR#34 save logged-in user details in cookies

9b2364b

PR#34 add js-cookie dependency to package.json

9903b11

mtreacy002 requested review from meenakshi-dhanani and a team July 2, 2020 13:49

ramitsawhney27 previously requested changes Jul 2, 2020

View reviewed changes

PR#34 add base api url constant and fix empty data check

496f614

ramitsawhney27 reviewed Jul 2, 2020

View reviewed changes

PR#34 refactor remainder of check to emptty object with _.isEmpty(obj)

42fd7dd

mtreacy002 added 3 commits July 3, 2020 20:17

PR#34 remove props, comments and refactor Navigation

ef4f341

PR#34 refactor to react-router-v5 on rendering and Nav tag Navigation

37d1efb

PR#34 Fix bugs - succcessful login now immediately show logout option

62a2b30

meenakshi-dhanani reviewed Jul 3, 2020

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

meenakshi-dhanani reviewed Jul 3, 2020

View reviewed changes

mtreacy002 and others added 14 commits July 4, 2020 11:02

PR#34 use conditional rendering for Navigation

9497a70

PR#34 abstract BASE_API constant to config file

7879c80

issue#30 test AuthContext as separrate funtion component

5f7b857

PR#34 fix AuthContext useState setting to get Cookies

f76c709

PR#34 add user into AuthContext value

69dac2a

PRR#34 use useEffect to persist login across pages

37e7298

PR#34 remove AuthConsumer

57f7526

PR#34 replace BASE_API syntax referencing

0687e34

#PR34 | Fix persisting context by removing hard reload and modifying …

e797cf3

…NavLink href as to

#PR34 | Remove underscore library and replace checks

79455f3

#PR34 | Add default value for context so that tests don't fail

ab732e7

Merge pull request #1 from meenakshi-dhanani/protected-page-fix

02d00dd

Protected page fix

PR#34 replace BASE_API syntax referencing

069f3a9

Resolved merge conflict

390a839

mtreacy002 changed the title ~~WIP: Feat: Issue#30 protected page and Persistent Token~~ Feat: Issue#30 protected page and Persistent Token Jul 8, 2020

meenakshi-dhanani approved these changes Jul 8, 2020

View reviewed changes

foongminwong approved these changes Jul 9, 2020

View reviewed changes

foongminwong merged commit a3ee4e9 into anitab-org:develop Jul 9, 2020

mtreacy002 deleted the issue#30-protected-page-v2 branch September 12, 2020 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Issue#30 protected page and Persistent Token #34

Feat: Issue#30 protected page and Persistent Token #34

mtreacy002 commented Jul 1, 2020 •

edited by isabelcosta

Loading

mtreacy002 commented Jul 2, 2020

ramitsawhney27 Jul 2, 2020

ramitsawhney27 Jul 2, 2020

mtreacy002 Jul 2, 2020

ramitsawhney27 Jul 2, 2020

mtreacy002 Jul 2, 2020

mtreacy002 commented Jul 2, 2020

ramitsawhney27 left a comment

mtreacy002 commented Jul 3, 2020 •

edited

Loading

mtreacy002 commented Jul 3, 2020

meenakshi-dhanani Jul 3, 2020

mtreacy002 Jul 4, 2020

meenakshi-dhanani commented Jul 3, 2020

mtreacy002 commented Jul 8, 2020

meenakshi-dhanani commented Jul 8, 2020

meenakshi-dhanani left a comment

meenakshi-dhanani commented Jul 8, 2020

meenakshi-dhanani commented Jul 9, 2020

foongminwong commented Jul 9, 2020

foongminwong left a comment

Feat: Issue#30 protected page and Persistent Token #34

Feat: Issue#30 protected page and Persistent Token #34

Conversation

mtreacy002 commented Jul 1, 2020 • edited by isabelcosta Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

mtreacy002 commented Jul 2, 2020

ramitsawhney27 Jul 2, 2020

Choose a reason for hiding this comment

ramitsawhney27 Jul 2, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 2, 2020

Choose a reason for hiding this comment

ramitsawhney27 Jul 2, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 2, 2020

Choose a reason for hiding this comment

mtreacy002 commented Jul 2, 2020

ramitsawhney27 left a comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 3, 2020 • edited Loading

mtreacy002 commented Jul 3, 2020

meenakshi-dhanani Jul 3, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 4, 2020

Choose a reason for hiding this comment

meenakshi-dhanani commented Jul 3, 2020

mtreacy002 commented Jul 8, 2020

meenakshi-dhanani commented Jul 8, 2020

meenakshi-dhanani left a comment

Choose a reason for hiding this comment

meenakshi-dhanani commented Jul 8, 2020

meenakshi-dhanani commented Jul 9, 2020

foongminwong commented Jul 9, 2020

foongminwong left a comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 1, 2020 •

edited by isabelcosta

Loading

mtreacy002 commented Jul 3, 2020 •

edited

Loading