KittenSquad/glewlwyd

Fork 0

Bug fixes for the Edit User modal #134

Merged

yrammos merged 2 commits from edit-user-modal-bugfix into master

2020-06-13 19:09:48 +02:00

yrammos commented

2020-06-12 22:42:46 +02:00

(Migrated from github.com)

Squashes two bugs linked to the Edit User modal of the admin webapp:

The OK button would be disabled, preventing user info revisions from being saved into the database. If I'm not mistaken, this is because the <button disable= condition was the logical complement (NOT) of what it should have been.
Upon clicking on CLOSE, any user info revisions would still be reflected in the User List, even though are meant to be discarded and are indeed not saved in the database (refreshing the browser proves the point). To address this, I duplicate the user state object, thereby sandboxing any user info revisions within the modal, and preventing their propagation to other components (i.e. copy-by-reference is replaced by copy-by-value logic). There might be more elegant solutions to this bug.

Squashes two bugs linked to the Edit User modal of the admin webapp: 1. The OK button would be disabled, preventing user info revisions from being saved into the database. If I'm not mistaken, this is because the `<button disable=` condition was the logical complement (`NOT`) of what it should have been. 2. Upon clicking on CLOSE, any user info revisions would still be reflected in the User List, even though are meant to be discarded and are indeed not saved in the database (refreshing the browser proves the point). To address this, I duplicate the `user` state object, thereby sandboxing any user info revisions within the modal, and preventing their propagation to other components (i.e. copy-by-reference is replaced by copy-by-value logic). There might be more elegant solutions to this bug.

babelouest commented

2020-06-12 23:45:44 +02:00

(Migrated from github.com)

Thanks again @yrammos !

Although I totally agree with the goal, I'd change the method for the 2nd point.
With your PR, it seems that when a change is saved in the modal, the component App would still have the original data.

Also defining an inner function inside the render() function is not elegant, especially since I'd apply the same change for all other components with modals: scheme, plugins, backends, etc.
I'll look around to find another way to fix 2.

Thanks again @yrammos ! Although I totally agree with the goal, I'd change the method for the 2nd point. With your PR, it seems that when a change is saved in the modal, the component App would still have the original data. Also defining an inner function inside the `render()` function is not elegant, especially since I'd apply the same change for all other components with modals: scheme, plugins, backends, etc. I'll look around to find another way to fix 2.

👍 1

yrammos commented

2020-06-13 11:17:56 +02:00

(Migrated from github.com)

@babelouest I agree that the second point is addressed here with some quick and dirty hackery.

Meanwhile I added one more commit to this PR. The "source" dropdown in the "new user" modal would be initialized with a default source, but the state would not. (Therefore the user would still need to select that source to actually have it stored in the state.) Not to risk messing around with states, here I'm opting for the most naive solution, i.e. to initialize the drop down with a blank source name; this should be enough for the user to realize that a source actually must be selected (albeit at the tiny usability expense of lacking a "default" dropdown option).

Hesitantly indeed, I'm also removing this.state.add from the OK <button disabled= condition. I think it prevented new user form submissions—kindly review and adjust if/as necessary. I'm not quite following the intended logic of the button state. Thanks as ever.

@babelouest I agree that the second point is addressed here with some quick and dirty hackery. Meanwhile I added one more commit to this PR. The "source" dropdown in the "new user" modal would be initialized with a default source, but the state would not. (Therefore the user would still need to select that source to actually have it stored in the state.) Not to risk messing around with states, here I'm opting for the most naive solution, i.e. to initialize the drop down with a blank source name; this should be enough for the user to realize that a source actually must be selected (albeit at the tiny usability expense of lacking a "default" dropdown option). Hesitantly indeed, I'm also removing `this.state.add` from the OK `<button disabled=` condition. I think it prevented new user form submissions—kindly review and adjust if/as necessary. I'm not quite following the intended logic of the button state. Thanks as ever.

babelouest commented

2020-06-13 15:23:46 +02:00

(Migrated from github.com)

Hello @yrammos ,

I've update your pull request in my branch yrammos-edit-user-modal-bugfix
Instead of deep copying data, I've used the good ol' JSON.stringify and JSON.parse to make a copy without copying the reference. Can you check that out?

Also, I'm wondering about the change in the disabled button attribute. Can you describe a use case where the button disabled attribute was buggy?

Hello @yrammos , I've update your pull request in my branch [yrammos-edit-user-modal-bugfix](https://github.com/babelouest/glewlwyd/tree/yrammos-edit-user-modal-bugfix) Instead of deep copying data, I've used the good ol' JSON.stringify and JSON.parse to make a copy without copying the reference. Can you check that out? Also, I'm wondering about the change in the `disabled` button attribute. Can you describe a use case where the button disabled attribute was buggy?

yrammos commented

2020-06-13 15:33:06 +02:00

(Migrated from github.com)

@babelouest thank you for developing this further. The disabled attribute is puzzling and honestly I don't quite recall the conditions. I remember that the button was disabled when I attempted to create a new user, but I might have been working on a "dirty" revision with other temporary changes that interfered. If everything looks well on your end without the disabled change, please feel free to omit it; I'll then report back if I can reproduce.

The JSON.stringify and JSON.parse method works quite well indeed, but will fail if the object contains a Date and other complex data. I'm sure you know this at least as well as I do, so the assumption is probably safe in this case, right? (Even "date" custom data would be stored as a string.)

@babelouest thank you for developing this further. The `disabled` attribute is puzzling and honestly I don't quite recall the conditions. I remember that the button was disabled when I attempted to create a new user, but I might have been working on a "dirty" revision with other temporary changes that interfered. If everything looks well on your end without the `disabled` change, please feel free to omit it; I'll then report back if I can reproduce. The `JSON.stringify` and `JSON.parse` method works quite well indeed, but will fail if the object contains a `Date` and other complex data. I'm sure you know this at least as well as I do, so the assumption is probably safe in this case, right? (Even "date" custom data would be stored as a string.)

babelouest commented

2020-06-13 15:50:02 +02:00

(Migrated from github.com)

That's the beauty of the data model, only simple data types, there's no complex data like Date or ByteArray. In fact, the data comes from the API response which is in JSON format. That's why I'm confident the JSON.stringify and JSON.parse will do the job :)

Concerning the disabled attribute, you're right, I've seen bugs too. I'll get back soon about that.

That's the beauty of the data model, only simple data types, there's no complex data like `Date` or `ByteArray`. In fact, the data comes from the API response which is in JSON format. That's why I'm confident the JSON.stringify and JSON.parse will do the job :) Concerning the `disabled` attribute, you're right, I've seen bugs too. I'll get back soon about that.

👍 1

yrammos commented

2020-06-13 18:07:28 +02:00

(Migrated from github.com)

Instead of deep copying data, I've used the good ol' JSON.stringify and JSON.parse to make a copy without copying the reference. Can you check that out?

@babelouest confirming that your fix for the CLOSE button issue works well on my end.

> Instead of deep copying data, I've used the good ol' JSON.stringify and JSON.parse to make a copy without copying the reference. Can you check that out? @babelouest confirming that your fix for the CLOSE button issue works well on my end.

👍 1

babelouest commented

2020-06-13 19:07:00 +02:00

(Migrated from github.com)

@yrammos , I beleive your disabled fix makes more sense than my old code, I'll use this change, if a bug appears after that, it'll be easier to fix anyway

@yrammos , I beleive your `disabled` fix makes more sense than my old code, I'll use this change, if a bug appears after that, it'll be easier to fix anyway

👍 1