Login failure does not generate a user visible error message (frontend)

[ots22]

A number of things could cause this, if the backend cannot be reached, for example.

When the login fails, the browser remains at the same location, but with the login clearly not having succeeded (login link still active). A message is written to the browser console.

Desired behaviour: write the message somewhere visible in the browser window (perhaps a modal).

[ots22]

Relevant code:

scivision/frontend/src/App.js

Lines 219 to 221 in 1d99794

	.catch((e) => {
	console.log(`Could not log in to GitHub. The reason was: ${e}`);
	})

[vimode]

Hey, I would like help with the frontend issues.

But I am unsure how to tackle this problem, the /login route isn't really a page displayed to the user(for long), so where can this error be displayed in the frontend? Back to the referrer page?

If so, one idea is to create a custom hook that passes the error messages to a context that displays the error in the frontend.

A dismissible Alert just below the Navbar or an autohiding Toast message could be an option.

Would appreciate your thoughts on this and feel free to correct me to whatever extend as I am still very much learning.

Thank you.

[ots22]

Hi @vimode,

It would be great if you could help us with the frontend.

I am also relatively new to frontend development/React (as are most people working on Scivision at the moment I think), but I hope we can figure many of these things out together.

On this issue, I think any of those approaches could work well. We use a Modal for error reporting elsewhere (see here), and should probably try to keep these consistent (but could change this everywhere, and even factor this out into our own error display component so we can try different styles).

For showing it on every page, you could include it in the top-level App component, like we do with the 'logged in' text here, and add a new state variable (with useState etc - there are several examples) to keep track of whether there was a recent failure.

Those are just some thoughts - do let me know if you have any other ideas or questions!

[vimode]

Hi @ots22

Thank you for replying so positively.

should probably try to keep these consistent (but could change this everywhere, and even factor this out into our own error display component so we can try different styles).

Understood, we certainly can improve these aspects and refactor later. For now we could just use the Modal as its already being used elsewhere.

For showing it on every page, you could include it in the top-level App component, like we do with the 'logged in' text here, and add a new state variable (with useState etc - there are several examples) to keep track of whether there was a recent failure.

Looking at that example. I just wanted to point out something generally, passing the setter function (setState) as a prop is not a great idea as it ties the setter function to the child component to some extend.
Anytime you want to update the that state in parent, the setter function being a prop for child will be sending that information to the child component too.
A commonly used approach for this is to create handler function in the parent component, to update that state and pass this function as a prop to the child component instead.

So something like

// parent component

//  handler function 
function githubLoggedInStatus () {
 set_gh_logged_in(true)
}
// prop
<Login githubLoggedStatus = {githubLoggedStatus} />

This keeps the control of the state within the parent within the component boundary.

In the current usage in App.js case, it might not be big problem as its not updated frequently but could get problematic.
I hope I was able to explain what I meant to, is this something that sounds like the way to go?

I am yet to go through the complete code properly. Will look into it this weekend and start working on it slowly starting with this issue of displaying error in the frontend.

I would also like to open an issue to suggest a project structure to organize it a bit. Some folders for components, views(pages), abstracted functions etc. I am sure you might have discussed this already at some point as the App.js is getting really long and could start getting entangled.

[ots22]

Hi @vimode, thanks for your reply and helpful comments.

.. [on passing setter as a prop] ..
I hope I was able to explain what I meant to, is this something that sounds like the way to go?

I wasn't aware of that, but that could be worthwhile. It's a shame it introduces an extra level of indirection in the code though. Do you happen to know of any good React resources about this, it's something I don't know that much about?

(I suppose it might be good to use contexts/some other way to manage the state eventually, but we only have a small number of these at the moment, so something we could come back to later.)

I would also like to open an issue to suggest a project structure to organize it a bit. Some folders for components, views(pages), abstracted functions etc. I am sure you might have discussed this already at some point as the App.js is getting really long and could start getting entangled.

There should be a PR reasonably soon with some better organisation (a bit overdue, as you can tell!) I could tag you on that when it comes through in case you wanted to have a look/give some feedback (only if you felt like it of course)?

[vimode]

it introduces an extra level of indirection in the code though

Yes, that it does. I stand corrected. This pattern ( passing setter to child) is okay with useContext, so its unnecessary to change in this case.
props being immutable, read-only and a new version of prop is passed for each render were mostly the reasons of my thinking. But now that I think of it, the state will not be frequently updated, Login not being a reusable nor a consumable component, and since the state is being used in the top level component anyway, re-renders is not an issue.

I suppose it might be good to use contexts/some other way to manage the state eventually..

I agree, there isn't much need to worry about state management as such and could be revisited later.
I thought it could be used for all the Errors as error handling is done in different ways throughout but I can see not all errors need to be showed in the frontend.

I could tag you on that when it comes through in case you wanted to have a look/give some feedback (only if you felt like it of course)

Yes, please do! I might not have much feedback to add. But for selfish reasons I would like to have a look at it, I personally would learn a lot. 😄

Thanks a lot @ots22 , this was really helpful.

[vimode]

Hi @ots22 I am kinda lost here and unable to really understand how the Login component works as I can't even get the error in the browser console as it is supposed to do.

I clicked on Login with Github and did not authorize the app expecting it to catch the error and log in the console but it just moves to the finally block and redirects back to the localhost.
Can you help me understand what am I missing here?

Is there some setup I need to do before I could able to run the app locally properly apart from installing the python libs?

Thank you.

[ots22]

Hi @vimode, thanks for having a look - I'll see if I can follow the code and summarize something here. It is possible that this could have been structured better, and the fact it isn't so clear suggests it could be improved (if you have a different structure in mind for this then please always feel free to propose changes).

Is there some setup I need to do before I could able to run the app locally properly apart from installing the python libs?

The frontend doesn't currently depend on python or the scivision python library at all. To run it locally, see here (it's basically a stock 'create-react-app', so should be as simple as npm install followed by npm start). These instructions aren't included in the 'maintainer' docs in the main documentation site, but they probably should be (I'll open an issue - thanks for surfacing that).

The GitHub Login should work locally too, assuming:

• it is running in development mode (npm start not npm build run)
• it is running on localhost port 3000 (the default).

There is a separate GitHub OAuth app, called 'scivision (local development)' that should ask you to grant it permission in order to log in.

[vimode]

Hey @ots22 Yes, with npm run start I was able to run the app locally, thats not the issue.

Its just that when I try to login, I was unable to get the expected error in the browser console when I denied authorization to Login so was wondering if there anything else I was missing. As you mentioned in another issue it does pollute the query params with the Github error when it navigates back but does not show the Login component nor the error as expected.

Thanks for the taking the time, but you don't have to look into it right away to reply back. Take your time. I can work on some other issues in the mean time. There are a few linting errors, React warnings, deprecated code usage, styling improvements etc that I noticed which I can work on in meantime.