-
Notifications
You must be signed in to change notification settings - Fork 69
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
[NEW] dummy auth model #171
Conversation
Dummy User Authentication Modeldummy_auth.mp4 |
This pull request fixes 1 alert when merging c09a95e into 13b5926 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 6525635 into b142610 - view on LGTM.com fixed alerts:
|
I've fixed almost all errors that were arising:
There remains only one error ( |
app/pages/api/auth/[...nextauth].js
Outdated
clientId: process.env.ROCKETCHAT_CLIENT_ID ?? "random", | ||
clientSecret: process.env.ROCKETCHAT_CLIENT_SECRET ?? "random", | ||
rocketChatUrl: process.env.ROCKETCHAT_URL ?? "http://localhost:3000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure empty strings aren't an issue here.
const login = () => { | ||
const dummy_user = dummyUserLogin(); | ||
setUser(dummy_user); | ||
sessionStorage.setItem("dummy_user", JSON.stringify(dummy_user)); | ||
} | ||
const logout = () => { | ||
setUser({}); | ||
sessionStorage.removeItem("dummy_user"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these to DummyLoginButton
and just pass the user
state. Pass these as handleLogin
and handleLogout
.
This pull request fixes 1 alert when merging f96a1d5 into 53d9c3d - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well @sidmohanty11 But what happened to the menu/TopNav? It would be good to have the menu change depending on the logged in state as well.
cms definitely initialized - otherwise there will be no element live. can you verify with a clean clone? I think you may have something added that has not been committed or initialized? @sidmohanty11 thx |
const createDummyUser = () => { | ||
return { | ||
id: 1, | ||
name: "dummy.cat", | ||
image: | ||
"https://user-images.githubusercontent.com/25859075/29918905-88dcc646-8e5c-11e7-81ec-242bc58dce1b.jpg", | ||
email: "[email protected]", | ||
emailVerified: false, | ||
phoneNumber: null, | ||
displayName: "dummy.cat", | ||
}; | ||
}; | ||
|
||
export const dummyUserLogin = () => { | ||
return createDummyUser(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get rid of dummyUserLogin
btw 👀
app/components/menubar.js
Outdated
{userCookie ? ( | ||
<Dropdown align="end" className={styles.dropdown_menu}> | ||
<Dropdown.Toggle as={CustomToggle} /> | ||
<Dropdown.Menu size="sm" title=""> | ||
<Dropdown.Header>RC4Community Profile</Dropdown.Header> | ||
<Dropdown.Item> | ||
<Link href={`/profile/${userCookie}`}> | ||
<a className={styles.dropdown_menu_item}>Profile</a> | ||
</Link> | ||
</Dropdown.Item> | ||
</Dropdown.Menu> | ||
</Dropdown> | ||
) : ( | ||
"" | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{userCookie ? ( | |
<Dropdown align="end" className={styles.dropdown_menu}> | |
<Dropdown.Toggle as={CustomToggle} /> | |
<Dropdown.Menu size="sm" title=""> | |
<Dropdown.Header>RC4Community Profile</Dropdown.Header> | |
<Dropdown.Item> | |
<Link href={`/profile/${userCookie}`}> | |
<a className={styles.dropdown_menu_item}>Profile</a> | |
</Link> | |
</Dropdown.Item> | |
</Dropdown.Menu> | |
</Dropdown> | |
) : ( | |
"" | |
)} | |
{userCookie && ( | |
<Dropdown align="end" className={styles.dropdown_menu}> | |
<Dropdown.Toggle as={CustomToggle} /> | |
<Dropdown.Menu size="sm" title=""> | |
<Dropdown.Header>RC4Community Profile</Dropdown.Header> | |
<Dropdown.Item> | |
<Link href={`/profile/${userCookie}`}> | |
<a className={styles.dropdown_menu_item}>Profile</a> | |
</Link> | |
</Dropdown.Item> | |
</Dropdown.Menu> | |
</Dropdown> | |
)} |
const handleLogin = () => { | ||
const dummy_user = dummyUserLogin(); | ||
setUser(dummy_user); | ||
sessionStorage.setItem("dummy_user", JSON.stringify(dummy_user)); | ||
}; | ||
const handleLogout = () => { | ||
setUser({}); | ||
sessionStorage.removeItem("dummy_user"); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to main component and pass them here. Mentioned this in the previous review, adding here to put it to the top. But, see my other comment on state.
const [isLoginUiOpen, setIsLoginUiOpen] = useState(false); | ||
const [user, setUser] = useState({}); | ||
|
||
useEffect(() => { | ||
const isStoredInSession = JSON.parse(sessionStorage.getItem("dummy_user")); | ||
if (isStoredInSession) { | ||
setUser(isStoredInSession); | ||
} | ||
},[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you're getting deeper into react, with this AND your gsoc project, try to advance your practical skills. Convert this to a custom hook like useAuth
:) or useDummyAuth
(prefer this name). Perfect situation to do that IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all createDummyUser
or validation functions in the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Ah, I just went with the conventional code structure of other auths. I will make the changes!
This pull request fixes 1 alert when merging fdfcf4c into 53d9c3d - view on LGTM.com fixed alerts:
|
This PR introduces,
Fixes #156