Uh oh!
There was an error while loading. Please reload this page.
Return proper boolean and do not save enabled state in db - #9111
Conversation
| if( $this->groupManager->isAdmin($currentLoggedInUser->getUID()) | ||
| || $this->groupManager->getSubAdmin()->isUserAccessible($currentLoggedInUser, $targetUserObject)) { | ||
| $data['enabled'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'enabled', 'true'); | ||
| $data['enabled'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'enabled', true) === true; |
There was a problem hiding this comment.
getUserValue will return a string, right?
So you need to compare the string in order to return a bool:$data['enabled'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'enabled', 'true') === 'true';
There was a problem hiding this comment.
It will return a string or the fallback which is a boolean, so true===true?true:false :)
There was a problem hiding this comment.
and if the string is 'true' your statement will be evaluated to false.
There was a problem hiding this comment.
it won't be true since we're not storing the enabled state in the database anymore if true.
Nonetheless, I need to think about the old config! So, fair point! :)
There was a problem hiding this comment.
so if the user set it to true we remove the complete row in the database and if the user set it to false we add it again with the value 'false'? Does this makes sense, sounds totally confusing to me. But maybe I miss some context here.
(Now I saw the second part of the code and re-read the headline, so my assumption was correct. It is probably to late for me to do reviews 😉 But I still think it is totally confusing and inconsistent with all other settings. I would just continue to store 'true' and 'false' and compare the strings to convert it back to bool)
There was a problem hiding this comment.
Well, a freshly created user is enabled, but doesn't have a userValue 'enabled' set.
It makes sense to create an entry when we disable one. But If we still keeps the userValue to 'true' in the db, it's where it gets confusing. We have users enabled without a userValue in the db and user enabled WITH a userValue in the db.
I say, let's put only one state, disabled: true or enabled by default
There was a problem hiding this comment.
We already discussed in the past that maybe apps (and the server) should write all config settings with the default value to the database on first start. I think this is still something we should explore. Because defining the default value in multiple areas in the code is super error prone. But that's a side topic.
Regarding this PR my opinion is that a database entry should be able to contain all supported values. If someone looks at the db (or the config.php by the way), sees a value 'false' and switched it to 'true', it should work. Having dark magic that any entry is false by comparing strings with bool in the code and having no entry at all is true is highly confusing and not consistent across Nextcloud. Already the idea comparing a string with a bool is a really bad one. Because X years down the road someone will look at the code, come to the conclusion that this is a bug (rightfully), fix it and break anything.
There was a problem hiding this comment.
Sure, I can do the other way around :)
Your point is good. My goal was not to have two different states for the enabled.
So, adding the config on user creation seems good to you?
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
444a667 to
eae5576CompareCodecov Report
@@ Coverage Diff @@## master #9111 +/- ##
=============================================
- Coverage 51.94% 31.57% -20.37% - Complexity 25297 25298 +1
=============================================
Files 1604 1604 Lines 95095 95087 -8 Branches 1388 1388 =============================================
- Hits 49397 30026 -19371 - Misses 45698 65061 +19363
|
skjnldsv
commented
Apr 9, 2018
Here you go @schiessle :) |
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
4979188 to
8b9bd37Compare
Since we store booleans as string in the db, let's check agains it so we also return a boolean in the json and not a string like "true".
Also, I introduced the removal of the "enabled" key when we enable a user since no config = true.