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

200 -redirect user to login if not logged in and tries to access '/project/new' #208

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

ALau2088
Copy link
Contributor

@ALau2088 ALau2088 commented Apr 25, 2019

  1. Add Private route to App.js line51.
  2. Add Private route component. If there is no session or user data redirect to login else continue to project/new. This is based on same logic as the CreateEventPage.js container.

Issue addressed

fixes #200

Screenshots (if appropriate):

Testing

@vercel
Copy link

vercel bot commented Apr 25, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://websiteone-fe-git-fork-alau2088-200-userredirect.agileventures1.now.sh

@mattwr18
Copy link
Contributor

hey thanks for putting this in @ALau2088! 😸
Can you run yarn fix before pushing up the code? it will fail our build if there are semi-colons, double quotes, etc.

cookies={this.props.cookies}
/>)
}}
<PrivateRoute exact path='/projects/new' component={NewProject} />
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a PrivateRoute here?
we can do something similar to the CreateEventPage where we save a user's lastLocation in our redux store, check if a user is logged in, and if not redirect them to the login page.


export default function NewProject() {
return (
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have a way to create new projects, please check out CreateProjectPage

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the issue was not clear enough? let me make update to explicitly state this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Just seeing the CreateProjectPage now. I will update my fork and make the requested changes shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

great @ALau2088, thanks for making the requested changes! 😸
just a slight change to the tests is needed see 👇

@mattwr18
Copy link
Contributor

@ALau2088 currently failing the build with this output

 Error: Uncaught [TypeError: Cannot read property 'pathname' of undefined]

can you try updating it similar to this?
https://github.com/AgileVentures/WebsiteOne-FE/blob/develop/src/tests/containers/CreateEventPage.test.js#L9

@vercel vercel bot requested a deployment to staging April 29, 2019 22:44 Abandoned
@vercel
Copy link

vercel bot commented Apr 29, 2019

Deployment failed with the following error:

You defined 1 build that did not match any source files (please ensure they are NOT defined in .nowignore):

@ALau2088
Copy link
Contributor Author

@mattwr18 . I made the requested edit and it passed the CircleCI test.

@ALau2088
Copy link
Contributor Author

ALau2088 commented May 3, 2019

@mattwr18 @aonomike . Hey guys, it just occurred to me that these changes were made to the CreateProjectPage prior to converting to redux-form. I could go ahead and implement this in the redux-form version of the components but before I do just wanted to check with you guys if this is the correct approach. Thanks in advance.

@chris110408
Copy link
Contributor

chris110408 commented May 3, 2019

@mattwr18 @aonomike @ALau2088

there is another option I just find is formsy-semantic-ui-react . https://github.com/zabute/formsy-semantic-ui-react
this one is easy to use and do not mutate the store by itself.
It is really easy to use and should able to handle all jobs required for this project

@mattwr18
Copy link
Contributor

mattwr18 commented May 4, 2019

hey @ALau2088, sorry I am just replying to this now, I have been real busy the last couple days.
I think for now we are going forward with Redux-Form, although we want to treat @chris110408 concerns as valid and explore the option of changing the direction and refactoring.

We need to talk about it in the next meeting, which will be Tuesday... up to you if you want to wait till then or not, but I think if you do want to go ahead and refactor, we can always open up a ticket later that you or someone else can work on.

At the moment, there are merge conflicts, so if you could rebase before you make any changes, it will make your life a bit easier.

@aonomike
Copy link
Contributor

aonomike commented May 6, 2019

Thanks @ALau2088 I ran this locally and looks great. Maybe we can have a session to add cypress tests?

@ALau2088
Copy link
Contributor Author

ALau2088 commented May 6, 2019

@aonomike Sure. Want to have session tomorrow Tuesday 5/7 10am-12pm Pacific Standard Time?
I am also available Thursday 5/9 8am-11am or during the React Mob MeetUp at 7am.

@joaopapereira
Copy link
Member

Fixes #200

@mattwr18
Copy link
Contributor

Any progress on this @ALau2088??

@ALau2088
Copy link
Contributor Author

Hey @mattwr18 . Thanks for the reminder. I added a setLastLocation test and redirect test and am ready to push it up to the PR but my branch files (CreateProjectPage.js and CreateProjectPage.test.js are behind the upstream files. My initial thought was to merge the upstream and branch but I wanted to see if you had any thoughts on going about this? Also I was curious what is the use of the setLastLocation action?

@mattwr18
Copy link
Contributor

you can run git rebase upstream/develop from your feature branch @ALau2088, then work through the merge conflicts

@ALau2088
Copy link
Contributor Author

Thanks @mattwr18 . The changes have been pushed up to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect user to Login page if not logged and attempts to create new project
5 participants