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.
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.
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:
|
Sing-Li
left a comment
There was a problem hiding this comment.
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: "dummyuser@rocket.chat", | ||
| emailVerified: false, | ||
| phoneNumber: null, | ||
| displayName: "dummy.cat", | ||
| }; | ||
| }; | ||
|
|
||
| export const dummyUserLogin = () => { | ||
| return createDummyUser(); | ||
| }; |
There was a problem hiding this comment.
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.
| {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.
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.
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.
Move all createDummyUser or validation functions in the same file.
There was a problem hiding this comment.
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