-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat: add example contact on first login #50156
base: master
Are you sure you want to change the base?
Conversation
b237d7a
to
3af4fc4
Compare
9101f07
to
979db68
Compare
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.
Tested and works.
b6cd5b0
to
3c3150b
Compare
Signed-off-by: Hamza Mahjoubi <[email protected]>
3c3150b
to
85f8d9f
Compare
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.
There are new classes but no new tests. Could you please add some unit tests at least?
|
||
public function getForm(): TemplateResponse { | ||
$enableDefaultContact = $this->config->getAppValue(Application::APP_ID, 'enableDefaultContact', 'no'); | ||
$this->initialState->provideInitialState('enableDefaultContact', $enableDefaultContact); |
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.
This is a code smell. Without looking at the method body of getForm, I wouldn't expect the method to have a side effect of filling an initial state. Try to move that out of the method, or rename the method.
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.
I agree, but it is commonly used across Nextcloud code base for this same purpose.
Checklist