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

Make CalDAV resource sync a service and expose as occ command #35388

Closed

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Nov 24, 2022

Summary

CalDAV resources and rooms are provided by apps. Currently the rooms and resources are read in a background job once an hour. Sometimes one wants to see the changes right away.

By moving the job logic into a reusable service we can also invoke it from an occ command.

This also drops the two unused events that aren't emitted anywhere

  • \OCP\Calendar\Resource\ForceRefreshEvent
  • \OCP\Calendar\Room\ForceRefreshEvent

TODO

  • Move logic into a service
  • Add occ command
  • Documentation

Checklist

@ChristophWurst ChristophWurst added enhancement 3. to review Waiting for reviews feature: dav pending documentation This pull request needs an associated documentation update feature: caldav Related to CalDAV internals labels Nov 24, 2022
@ChristophWurst ChristophWurst added this to the Nextcloud 26 milestone Nov 24, 2022
@ChristophWurst ChristophWurst self-assigned this Nov 24, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.


class CalendarResourcesRoomsSyncService {

/** @var IResourceManager */

Check notice

Code scanning / Psalm

DeprecatedClass

Class OCP\Calendar\Resource\IManager is marked as deprecated
/** @var IResourceManager */
private $resourceManager;

/** @var IRoomManager */

Check notice

Code scanning / Psalm

DeprecatedClass

Class OCP\Calendar\Room\IManager is marked as deprecated
/** @var CalDavBackend */
private $calDavBackend;

public function __construct(IResourceManager $resourceManager,

Check notice

Code scanning / Psalm

DeprecatedClass

Class OCP\Calendar\Resource\IManager is marked as deprecated
private $calDavBackend;

public function __construct(IResourceManager $resourceManager,
IRoomManager $roomManager,

Check notice

Code scanning / Psalm

DeprecatedClass

Class OCP\Calendar\Room\IManager is marked as deprecated
* Run background-job for one specific backendManager
* either ResourceManager or RoomManager
*
* @param IResourceManager|IRoomManager $backendManager

Check notice

Code scanning / Psalm

DeprecatedClass

Class OCP\Calendar\Resource\IManager is marked as deprecated
string $dbTableMetadata,
string $foreignKey,
string $principalPrefix): void {
$backends = $backendManager->getBackends();

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\Calendar\Resource\IManager::getBackends has been marked as deprecated
continue;
}

$id = $this->addToCache($dbTable, $backendId, $resource);

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 3 of OCA\DAV\CalDAV\CalendarResourcesRoomsSyncService::addToCache cannot be null, possibly null value provided
continue;
}

$this->updateCache($dbTable, $id, $resource);

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 3 of OCA\DAV\CalDAV\CalendarResourcesRoomsSyncService::updateCache cannot be null, possibly null value provided
$this->syncService = $syncService;
}

protected function configure() {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\DAV\Command\SyncResourcesRooms::configure does not have a return type, expecting void
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.

👍 makes sense

I guess we can't use the new occ background-job:execute instead ?

cc @come-nc

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I think using background-job:execute works too in this case, given the job is always planned.

But that requires using background-job:list first to get the id, so I have no real problem with having a specific command for it.
That said we should look into improving background-job:execute to be able to run jobs by class name, and background-job:list to be able so search for jobs without knowing the exact name.

Comment on lines +48 to +52
protected function execute(InputInterface $input, OutputInterface $output): int {
$this->syncService->sync();

return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid silent commands, at least output something in verbose mode.

Comment on lines +35 to +40
private CalendarResourcesRoomsSyncService $syncService;

public function __construct(CalendarResourcesRoomsSyncService $syncService) {
parent::__construct();
$this->syncService = $syncService;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private CalendarResourcesRoomsSyncService $syncService;
public function __construct(CalendarResourcesRoomsSyncService $syncService) {
parent::__construct();
$this->syncService = $syncService;
}
public function __construct(
private CalendarResourcesRoomsSyncService $syncService,
) {
parent::__construct();
}

Use the force Luke (if you do not need backports)

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@ChristophWurst ChristophWurst added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels May 10, 2023
@ChristophWurst ChristophWurst removed this from the Nextcloud 27 milestone May 10, 2023
@ChristophWurst ChristophWurst marked this pull request as draft May 10, 2023 08:54
@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@skjnldsv skjnldsv added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Feb 21, 2024
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 6, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@skjnldsv skjnldsv deleted the enhancement/caldav-resources-sync-command branch August 30, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement feature: caldav Related to CalDAV internals feature: dav pending documentation This pull request needs an associated documentation update
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

5 participants