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.

Sign up for GitHub

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

Jump to bottom

dav: Converter & SyncService shall not use private AccountManager #26922

Merged
merged 2 commits into from
May 12, 2021
Merged

dav: Converter & SyncService shall not use private AccountManager #26922

merged 2 commits into from
May 12, 2021

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented May 7, 2021 •
edited
Loading

  • also updates data structure in test

helps with #26866

@blizzz blizzz added 3. to review Waiting for reviews technical debt labels May 7, 2021
@blizzz blizzz added this to the Nextcloud 22 milestone May 7, 2021
@blizzz blizzz force-pushed the techdebt/noid/dav-no-private-class branch 2 times, most recently from d1d1bc6 to 04edd06 Compare May 7, 2021 22:17
@blizzz
dav: do not use private AccountManager
04edd06 
- also updates data structure in test

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented May 7, 2021

CI is yet unhappy

There was 1 failure:

  1. OCA\DAV\Tests\unit\CardDAV\SyncServiceTest::testUpdateAndDeleteUser with data set #0 (true, 1, 1, 1)
    OCA\DAV\CardDAV\CardDavBackend::deleteCard(-1, 'unittest:test-user.vcf') was not expected to be called more than once.

/drone/src/apps/dav/lib/CardDAV/SyncService.php:304
/drone/src/apps/dav/tests/unit/CardDAV/SyncServiceTest.php:178

Checking this next week

@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 7, 2021
@blizzz
Copy link
Member Author

blizzz commented May 10, 2021

CI is yet unhappy

There was 1 failure:

  1. OCA\DAV\Tests\unit\CardDAV\SyncServiceTest::testUpdateAndDeleteUser with data set #0 (true, 1, 1, 1)
    OCA\DAV\CardDAV\CardDavBackend::deleteCard(-1, 'unittest:test-user.vcf') was not expected to be called more than once.

/drone/src/apps/dav/lib/CardDAV/SyncService.php:304
/drone/src/apps/dav/tests/unit/CardDAV/SyncServiceTest.php:178

Checking this next week

Uargh, that's not a unit test. Unmocked Converter. 😢

PVince81
PVince81 reviewed May 11, 2021
View reviewed changes
apps/dav/lib/CardDAV/Converter.php
@@ -65,43 +56,39 @@ public function createCardFromUser(IUser $user) {

$publish = false;

if ($image !== null && isset($userData[IAccountManager::PROPERTY_AVATAR])) {
Copy link
Member

Choose a reason for hiding this comment

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

where did this one go ? obsolete ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is obsolete, because we run over the set attributes, so no need to check for the presence anymore. Looked strange anyway.

PVince81
PVince81 approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@blizzz
remove private AccountManager from SyncService
665ffbd 
and fix test

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented May 11, 2021

Checking this next week

Uargh, that's not a unit test. Unmocked Converter. cry

Done minimally in second attempt. Was too ambitious at first. Too big a yak.

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 11, 2021
@blizzz blizzz requested a review from skjnldsv May 11, 2021 21:44
@blizzz blizzz changed the title dav: Converter shall not use private AccountManager dav: Converter & SyncService shall not use private AccountManager May 11, 2021
@rullzer rullzer merged commit a2a9646 into master May 12, 2021
@rullzer rullzer deleted the techdebt/noid/dav-no-private-class branch May 12, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: dav technical debt
Projects
None yet
Milestone
Nextcloud 22
Development

Successfully merging this pull request may close these issues.
4 participants
@blizzz @PVince81 @skjnldsv @rullzer