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

[NEW] dummy auth model #171

Merged
merged 4 commits into from
Jul 14, 2022
Merged

[NEW] dummy auth model #171

merged 4 commits into from
Jul 14, 2022

Conversation

sidmohanty11
Copy link
Contributor

@sidmohanty11 sidmohanty11 commented Jun 17, 2022

This PR introduces,

  • a dummy authentication model
  • removes firebase error
  • fixes all the console errors for a smooth development experience

Fixes #156

@sidmohanty11
Copy link
Contributor Author

Dummy User Authentication Model

dummy_auth.mp4

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request fixes 1 alert when merging c09a95e into 13b5926 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 3, 2022

This pull request fixes 1 alert when merging 6525635 into b142610 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@sidmohanty11
Copy link
Contributor Author

I've fixed almost all errors that were arising:

  • firebase error
  • slideCount error

There remains only one error (<button> inside <button>) which is coming from the Menubar component because we are using Navbar.Toggle but inside for styling, we are using another button. This should be fixed with the new Menubar component which is being worked on in this PR.

image

@sidmohanty11 sidmohanty11 changed the title [WIP] dummy auth model [NEW] dummy auth model Jul 3, 2022
app/lib/auth/RocketChatOAuthProvider.js Outdated Show resolved Hide resolved
Comment on lines 23 to 25
clientId: process.env.ROCKETCHAT_CLIENT_ID ?? "random",
clientSecret: process.env.ROCKETCHAT_CLIENT_SECRET ?? "random",
rocketChatUrl: process.env.ROCKETCHAT_URL ?? "http://localhost:3000"
Copy link
Member

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.

app/components/auth/dummy/ui/DummyLoginButton.js Outdated Show resolved Hide resolved
app/components/auth/dummy/lib/function.js Outdated Show resolved Hide resolved
Comment on lines 6 to 14
const login = () => {
const dummy_user = dummyUserLogin();
setUser(dummy_user);
sessionStorage.setItem("dummy_user", JSON.stringify(dummy_user));
}
const logout = () => {
setUser({});
sessionStorage.removeItem("dummy_user");
}
Copy link
Member

@debdutdeb debdutdeb Jul 3, 2022

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.

app/components/auth/dummy/ui/DummyLoginUI.js Outdated Show resolved Hide resolved
app/components/menubar.js Outdated Show resolved Hide resolved
app/components/menubar.js Outdated Show resolved Hide resolved
@sidmohanty11 sidmohanty11 requested a review from debdutdeb July 4, 2022 19:21
@lgtm-com
Copy link

lgtm-com bot commented Jul 4, 2022

This pull request fixes 1 alert when merging f96a1d5 into 53d9c3d - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

Copy link
Member

@Sing-Li Sing-Li left a comment

Choose a reason for hiding this comment

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

image

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.

@sidmohanty11
Copy link
Contributor Author

It is there in my case.
image

Is it due to cms uninitialized? But that should break the app, right? Can you check with running cms?

@sidmohanty11 sidmohanty11 requested a review from Sing-Li July 6, 2022 07:35
@Sing-Li
Copy link
Member

Sing-Li commented Jul 7, 2022

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

Comment on lines 1 to 16
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();
};
Copy link
Member

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 👀

Comment on lines 102 to 116
{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>
) : (
""
)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{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>
)}

Comment on lines 6 to 14
const handleLogin = () => {
const dummy_user = dummyUserLogin();
setUser(dummy_user);
sessionStorage.setItem("dummy_user", JSON.stringify(dummy_user));
};
const handleLogout = () => {
setUser({});
sessionStorage.removeItem("dummy_user");
};
Copy link
Member

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.

Comment on lines 7 to 15
const [isLoginUiOpen, setIsLoginUiOpen] = useState(false);
const [user, setUser] = useState({});

useEffect(() => {
const isStoredInSession = JSON.parse(sessionStorage.getItem("dummy_user"));
if (isStoredInSession) {
setUser(isStoredInSession);
}
},[])
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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!

@sidmohanty11 sidmohanty11 requested a review from debdutdeb July 9, 2022 07:25
@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2022

This pull request fixes 1 alert when merging fdfcf4c into 53d9c3d - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@sidmohanty11
Copy link
Contributor Author

Did a clean clone and install, the top nav works for me.
image

@Sing-Li Sing-Li merged commit 38dffd9 into RocketChat:master Jul 14, 2022
Dnouv pushed a commit to Dnouv/RC4Community that referenced this pull request Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TODO] Create a simple-local-auth component to improve out-of-box DevEx
3 participants