-
Notifications
You must be signed in to change notification settings - Fork 40
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
added "root" attribute to user entity #1630
Conversation
☔ The latest upstream changes (presumably dc70e8b) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -470,6 +470,7 @@ public function removeLearnerIlmSession(IlmSessionInterface $learnerIlmSession); | |||
public function getLearnerIlmSessions(); | |||
|
|||
/** | |||
<<<<<<< 198377f486608690bd5286f036e4f848848c8c58 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎
According to @stopfstedt this needs to have protection in the controller to prevent anyone from setting the root attr for users. |
@jrjohnson please re-review, esp. the changes to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only comment is on the names of the set/unset root commands. That may only be the way I'm thinking about it though.
UserController looks good to me. Great tests.
/** | ||
* @var string | ||
*/ | ||
const COMMAND_NAME = 'ilios:maintenance:set-root-user'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set-root-user implies to me that there is only one. should probably be called add-root-user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed.
/** | ||
* @var string | ||
*/ | ||
const COMMAND_NAME = 'ilios:maintenance:unset-root-user'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove-root-user would make more sense otherwise maybe it removes everyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed.
@stopfstedt failing test here. |
ugh, searched/replaced without thinking. fixed. |
refs #1623