Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Jan 13, 2025

Checklist

@hamza221 hamza221 marked this pull request as ready for review January 21, 2025 12:30
@hamza221 hamza221 self-assigned this Jan 21, 2025
@hamza221 hamza221 added enhancement 3. to review Waiting for reviews feature: carddav Related to CardDAV internals labels Jan 21, 2025
@hamza221 hamza221 force-pushed the feat/cardav-example-contact branch from b237d7a to 3af4fc4 Compare January 22, 2025 12:47
@hamza221 hamza221 marked this pull request as draft January 23, 2025 09:45
@hamza221 hamza221 added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 23, 2025
@hamza221 hamza221 marked this pull request as ready for review January 23, 2025 14:33
@hamza221 hamza221 requested a review from st3iny January 23, 2025 14:33
apps/dav/src/views/ExampleContactSettings.vue Outdated Show resolved Hide resolved
apps/dav/src/views/ExampleContactSettings.vue Outdated Show resolved Hide resolved
apps/dav/src/views/ExampleContactSettings.vue Outdated Show resolved Hide resolved
apps/dav/lib/Settings/ExampleContentSettings.php Outdated Show resolved Hide resolved
apps/dav/src/views/ExampleContactSettings.vue Outdated Show resolved Hide resolved
apps/dav/lib/Controller/ExampleContentController.php Outdated Show resolved Hide resolved
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works.

apps/dav/lib/Controller/ExampleContentController.php Outdated Show resolved Hide resolved
@st3iny st3iny added this to the Nextcloud 32 milestone Jan 27, 2025
@st3iny st3iny added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 27, 2025
@hamza221 hamza221 force-pushed the feat/cardav-example-contact branch from b6cd5b0 to 3c3150b Compare January 28, 2025 11:26
@hamza221 hamza221 force-pushed the feat/cardav-example-contact branch from 3c3150b to 85f8d9f Compare January 28, 2025 11:27
Copy link
Member

@ChristophWurst ChristophWurst left a 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);
Copy link
Member

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.

Copy link
Contributor Author

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.

apps/dav/src/views/ExampleContactSettings.vue Show resolved Hide resolved
apps/dav/src/views/ExampleContactSettings.vue Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: carddav Related to CardDAV internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an example contact
3 participants