Ui bugfixes #137

2020-06-16T13:31:15+02:00

yrammos commented

2020-06-16 13:31:15 +02:00

(Migrated from github.com)

Hello @babelouest,

Your UI refactoring looks excellent. Thank you!

Meanwhile here is a supplementary PR (on top of your yrammos-ui-tidbits branch) for two bugs that are apparently inherited from master but become more apparent after these UI changes.

`20f5db0e`

This is a fix for two independent navbar bugs:

Profile app bug

Log into the Profile app with at least two users.
Log out as one of these users.
The navbar does not display the remaining session user(s). This is deceptive; clicking on the navbar Log In button will confirm they are ready to "continue" (green button).

There is a line in Profile/App.js that I'm not sure about and have marked accordingly in the comments.

Admin app bug

Log into the Admin app with exactly two users.
Log out as one of these users.
Log out as the other user.
The navbar still displays the last user. This is deceptive; clicking on the navbar Log In button will only produce the log in form (no user is active in the session).

`287c4ec1`

This is a fix to an insidious "Continue" button bug:

Log into either the Profile or the Admin app with two users.
Log out as one of these users. The navbar should now display the second (still active) user.
Click on the Log In button.
Click on the green "Continue" button.
Bug: the user is not allowed to log in.
Click on the Log In button again.
Select the same user in the Change User dropdown.
Now click on "Continue" again.
The user is logged in (as expected).

This bug fix ensures that this spurious "Change User" operation is no longer necessary.

Hello @babelouest, Your UI refactoring looks excellent. Thank you! Meanwhile here is a supplementary PR (on top of your `yrammos-ui-tidbits` branch) for two bugs that are apparently inherited from `master` but become more apparent after these UI changes. ### 20f5db0e This is a fix for two independent navbar bugs: #### Profile app bug 1. Log into the Profile app with at least two users. 2. Log out as one of these users. 3. The navbar does not display the remaining session user(s). This is deceptive; clicking on the navbar Log In button will confirm they are ready to "continue" (green button). There is a line in `Profile/App.js` that I'm not sure about and have marked accordingly in the comments. #### Admin app bug 1. Log into the Admin app with exactly two users. 2. Log out as one of these users. 3. Log out as the other user. 4. The navbar still displays the last user. This is deceptive; clicking on the navbar Log In button will only produce the log in form (no user is active in the session). ### 287c4ec1 This is a fix to an insidious "Continue" button bug: 1. Log into either the Profile or the Admin app with two users. 2. Log out as one of these users. The navbar should now display the second (still active) user. 3. Click on the Log In button. 4. Click on the green "Continue" button. 5. Bug: the user is not allowed to log in. 6. Click on the Log In button again. 7. Select the same user in the Change User dropdown. 8. Now click on "Continue" again. 9. The user is logged in (as expected). This bug fix ensures that this spurious "Change User" operation is no longer necessary.

babelouest (Migrated from github.com) reviewed

2020-06-16 13:31:15 +02:00

babelouest (Migrated from github.com) requested changes

2020-06-16 13:36:42 +02:00

webapp-src/src/Admin/App.js

					
				@ -104,6 +104,8 @@ class App extends Component {

				        this.setState({loggedIn: message.message}, () => {

better off using one this.setState rather than 2

better off using one `this.setState` rather than 2

webapp-src/src/Admin/App.js

console.log are not necessary

`console.log` are not necessary

webapp-src/src/Login/Buttons.js

As I said in this comment: github.com/yrammos/glewlwyd@6dccebdb81 (r39940743) , you should not call apiManager.glewlwydRequest inside the render() function. What are you trying to achieve here?

As I said in this comment: https://github.com/yrammos/glewlwyd/commit/6dccebdb81856abd00774d3004e46fe2144b4871#r39940743 , you should not call `apiManager.glewlwydRequest` inside the `render()` function. What are you trying to achieve here?

webapp-src/src/Profile/App.js

					
				@ -64,7 +64,8 @@ class App extends Component {

				      } else if (message.type === 'loggedIn') {

Those 3 setState calls should be merged in one, especially since a setState is asynchronous, this could lead to undefined behavior...

Those 3 `setState` calls should be merged in one, especially since a setState is asynchronous, this could lead to undefined behavior...

yrammos commented

2020-06-16 13:53:47 +02:00

(Migrated from github.com)

@babelouest there was a confusion of commits. I'll review your comments and revise as necessary. Thank you.

yrammos (Migrated from github.com) reviewed

2020-06-16 14:02:22 +02:00

webapp-src/src/Login/Buttons.js

@babelouest thank you for the review.

The reproduction vector is described in the PR. To describe the symptom (not the cause) in brief: the green "Continue" button does not actually continue if the current user was made active automatically after another user logged out.

@babelouest thank you for the review. The reproduction vector is described in the PR. To describe the symptom (not the cause) in brief: the green "Continue" button does not actually continue if the current user was made active automatically after another user logged out.

yrammos commented

2020-06-16 14:07:05 +02:00

(Migrated from github.com)

Updated. If you agree on the principle, I'll move the API call outside of render() (shameful mistake).

Updated. If you agree on the principle, I'll move the API call outside of `render()` (shameful mistake).

babelouest commented

2020-06-16 14:29:58 +02:00

(Migrated from github.com)

@yrammos , instead of calling the API inside the render, you should probably look around the navbar event when the user clicks logout, after that the event should be propagated to App.js. There, a call to this.fetchProfile(); should be sufficient to update the buttons.

Also, please use the Resolve conversation button when you commit changes that fixes the reviews, it'll be easier to follow.

@yrammos , instead of calling the API inside the render, you should probably look around the navbar event when the user clicks logout, after that the event should be propagated to App.js. There, a call to `this.fetchProfile();` should be sufficient to update the buttons. Also, please use the `Resolve conversation` button when you commit changes that fixes the reviews, it'll be easier to follow.

yrammos commented

2020-06-16 14:56:57 +02:00

(Migrated from github.com)

@yrammos , instead of calling the API inside the render, you should probably look around the navbar event when the user clicks logout, after that the event should be propagated to App.js. There, a call to this.fetchProfile(); should be sufficient to update the buttons.

@babelouest this.fetchProfile() is a method of Profile/App.js, whereas the bug is manifest just also when logging in from the Admin app (which doesn't have a fetchProfile in the first place). I think that the Login app is the real "owner" of the bug: once again, clicking on "Change User => [Username]" to select the already active user resolves the problem. (Of course the user shouldn't have to do this.) Doesn't this symptom the locate the problem, conceptually, in the Login page? I'll try moving the fix from render() to the constructor of Login/App.js.

> @yrammos , instead of calling the API inside the render, you should probably look around the navbar event when the user clicks logout, after that the event should be propagated to App.js. There, a call to `this.fetchProfile();` should be sufficient to update the buttons. @babelouest `this.fetchProfile()` is a method of `Profile/App.js`, whereas the bug is manifest just also when logging in from the Admin app (which doesn't have a `fetchProfile` in the first place). I think that the Login app is the real "owner" of the bug: once again, clicking on "Change User => [Username]" to select *the already active user* resolves the problem. (Of course the user shouldn't have to do this.) Doesn't this symptom the locate the problem, conceptually, in the Login page? I'll try moving the fix from `render()` to the constructor of `Login/App.js`.

yrammos (Migrated from github.com) reviewed

2020-06-16 15:02:19 +02:00

webapp-src/src/Profile/App.js

					
				@ -64,7 +64,8 @@ class App extends Component {

				      } else if (message.type === 'loggedIn') {

@babelouest I am unsure about the last of these three setState statements (which I will merge into one): is there supposed to be a 1-1 correspondence between profileList and schemeList members? In other words, when profileList[0] logs out and is popped from profileList, should schemeList[0] also be popped from schemeList?

@babelouest I am unsure about the last of these three `setState` statements (which I will merge into one): is there supposed to be a 1-1 correspondence between `profileList` and `schemeList` members? In other words, when `profileList[0]` logs out and is popped from `profileList`, should `schemeList[0]` also be popped from `schemeList`?

babelouest commented

2020-06-16 15:20:21 +02:00

(Migrated from github.com)

My bad, I mixed up admin and profile page, but the pages architecture are similar with an App.js and bottom up message transmission. Let me know if you need help understanding the code

babelouest (Migrated from github.com) reviewed

2020-06-16 15:23:41 +02:00

webapp-src/src/Profile/App.js

					
				@ -64,7 +64,8 @@ class App extends Component {

				      } else if (message.type === 'loggedIn') {

If you log out from a component, you should send a message to App component so it will refresh The status of all subcomponents.

yrammos (Migrated from github.com) reviewed

2020-06-16 15:47:05 +02:00

webapp-src/src/Profile/App.js

					
				@ -64,7 +64,8 @@ class App extends Component {

				      } else if (message.type === 'loggedIn') {

Right, I understand that @babelouest, in fact I’m working precisely on the message receiver here. I believe that the original code incorrectly zeroes out the entire profileList whereas only the last active user should be removed (profileList[0]). I’ve checked this part of the fix quite a bit and it looks robust. My question is whether the same fix should be repeated for schemeList as well.

Right, I understand that @babelouest, in fact I’m working precisely on the message receiver here. I believe that the original code incorrectly zeroes out the entire `profileList` whereas only the last active user should be removed (`profileList[0]`). I’ve checked this part of the fix quite a bit and it looks robust. My question is whether the same fix should be repeated for `schemeList` as well.

yrammos (Migrated from github.com) reviewed

2020-06-16 18:27:54 +02:00

webapp-src/src/Profile/App.js

					
				@ -64,7 +64,8 @@ class App extends Component {

				      } else if (message.type === 'loggedIn') {

I think I figured it out: schemeList is the current user's list of authentication schemes, so it should be indeed zeroed out each time a user logs out. Declaring this resolved and I assume it will raise a red flag with you if my understanding is incorrect.

I think I figured it out: `schemeList` is the current user's list of authentication schemes, so it should be indeed zeroed out each time a user logs out. Declaring this resolved and I assume it will raise a red flag with you if my understanding is incorrect.

yrammos commented

2020-06-16 19:55:30 +02:00

(Migrated from github.com)

@babelouest the API call is now moved outside render().

My informal testing suggests that the PR fixes the two bugs mentioned above but creates an issue with the red Log Out button of Login/Buttons.js, which does logs out but then immediately logs back in.

I confess having difficulties navigating the code because all your components are stateful. In my modest experience with ReactJS only a minimal number of components are stateful and the rest rely on immutable props. Would be thankful if you could take a look at the red button issue.

@babelouest the API call is now moved outside `render()`. My informal testing suggests that the PR fixes the two bugs mentioned above but creates an issue with the red Log Out button of `Login/Buttons.js`, which does logs out but then immediately logs back in. I confess having difficulties navigating the code because all your components are stateful. In my modest experience with ReactJS only a minimal number of components are stateful and the rest rely on immutable props. Would be thankful if you could take a look at the red button issue.

babelouest commented

2020-06-16 20:55:35 +02:00

(Migrated from github.com)

@yrammos , I agree my react code isn't very clean in terms of statefulness, I'm no expert either.

But I'm trying to understand where you at with your pull requests. You closed #136 without having it merged in my master, is it for a reason?

@yrammos , I agree my react code isn't very clean in terms of statefulness, I'm no expert either. But I'm trying to understand where you at with your pull requests. You closed #136 without having it merged in my master, is it for a reason?

yrammos commented

2020-06-16 21:00:40 +02:00

(Migrated from github.com)

Sincere apologies, @babelouest—closed by accident. (Working under tight deadline, fingers running ahead of mind.)

babelouest commented

2020-06-16 21:05:13 +02:00

(Migrated from github.com)

@yrammos , don't worry, but especially since you have a deadline, let's focus on one step at a time, when we'll complete this PR, we'll take care of the next one

yrammos commented

2020-06-16 21:08:15 +02:00

(Migrated from github.com)

Grateful for your help @babelouest. Just to note the deadline is mine—I don’t mean to impose !

yrammos commented

2020-06-16 22:39:31 +02:00

(Migrated from github.com)

@babelouest this PR has a slew of problems and, you're right, better wait until past PR have been merged. Closing for now; I'll resubmit as an issue after #136 has been merged.

babelouest commented

2020-06-17 00:10:02 +02:00

(Migrated from github.com)

@yrammos , btw, I've found a bug in the backend related to the logout button, I fixed it in my branch. Also, I've figured how to fix the UI bug too, it'll be easy as well, it only requires to reload the user list when the logout button is clicked. With the backend fix it'll work.

👍 1