Skip to content

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented May 30, 2025

Summary

  • Expose whether calendar app is enabled to be able to link to it
  • Allows Talk Desktop client to pick it up

Checklist

@Antreesy Antreesy added this to the Nextcloud 32 milestone May 30, 2025
@Antreesy Antreesy self-assigned this May 30, 2025
@Antreesy Antreesy requested a review from a team as a code owner May 30, 2025 12:27
@Antreesy Antreesy added 3. to review Waiting for reviews feature: caldav Related to CalDAV internals labels May 30, 2025
@Antreesy Antreesy requested review from ArtificialOwl, skjnldsv and yemkareems and removed request for a team May 30, 2025 12:27
@Antreesy Antreesy force-pushed the fix/noid/expose-calendar-enabled branch from 1e0115b to f113310 Compare May 30, 2025 12:32
@Antreesy Antreesy requested review from a team and ChristophWurst May 30, 2025 12:32
@nickvergessen
Copy link
Member

/backport to stable31

@Antreesy Antreesy removed the request for review from a team May 30, 2025 12:33
@skjnldsv
Copy link
Member

I would honestly just create a dedicated list of enabled apps for the logged in user.
Seems more generic and everyone could benefit from it :)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/expose-calendar-enabled branch from f113310 to bf6b147 Compare May 30, 2025 13:19
@Antreesy
Copy link
Contributor Author

I would honestly just create a dedicated list of enabled apps for the logged in user

Sounds like a good idea, but for web applications it should be enough to rely on _oc_appswebroots, I think? Otherwise it can be done selectively to not increase a load on each webpage request, (e.g. if you have 100+ apps enabled, and eventually it's not going to be used by any app)

I don't even need this in Talk web page (we have initial state there), it just a bypass for Talk Desktop client to get that data from server capabilities periodically

@skjnldsv
Copy link
Member

Sounds like a good idea, but for web applications it should be enough to rely on _oc_appswebroots, I think?

I don't think so, apps webroots doesn't mean the user have rights to use that app? 🤔

@SystemKeeper
Copy link
Contributor

I don't think so, apps webroots doesn't mean the user have rights to use that app? 🤔

From

if ($this->currentUser === null) {
$apps = $this->appManager->getEnabledApps();
} else {
$apps = $this->appManager->getEnabledAppsForUser($this->currentUser);
}
foreach ($apps as $app) {
try {
$apps_paths[$app] = $this->appManager->getAppWebPath($app);
} catch (AppPathNotFoundException $e) {
$apps_paths[$app] = false;
}
}

I would assume it does? 🤔 Since it checks

public function getEnabledAppsForUser(IUser $user) {
$apps = $this->getEnabledAppsValues();
$appsForUser = array_filter($apps, function ($enabled) use ($user) {
return $this->checkAppForUser($enabled, $user);
});
return array_keys($appsForUser);
}

?

But a general list would be nice, yea, we have dedicated capabilities in a few apps for that right now.

Antreesy added 2 commits May 31, 2025 11:40
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@susnux
Copy link
Contributor

susnux commented Jun 1, 2025

IMHO this does not belong to dav.
Instead put this is the calendar app, its already a mess that we cross link from app to other app.

You can then check if calendar capability is available or not.

@nickvergessen
Copy link
Member

You can then check if calendar capability is available or not.

Makes also sense there, but it doesn't have any yet.

@ChristophWurst
Copy link
Member

Is the capabilities endpoint always specific to a user or is it supposed to be global for the instance? I also see enable app for group as a special case that could be tricky to handle if we mix this up. Otherwise capability or general list of apps for the current user sounds good 👍

@Antreesy
Copy link
Contributor Author

Antreesy commented Jun 2, 2025

Made an attempt to move it to core capabilities, output is a list of enabled apps. I wonder if there is an issue in exposing this list?

  • As 'guests' app account, I see the same list of apps - although some should be blocked?
  • As public link visitor, I have no account - I see all the apps on the instance?
  • as federated user - depends on how a proxying requests handles it (didn't test)?

@susnux
Copy link
Contributor

susnux commented Jun 2, 2025

output is a list of enabled apps. I wonder if there is an issue in exposing this list?

If it is the same as on the JS config, then no. In general I think everything in the JS config should be probably a capability to allow using this also in other locations than the webUI (e.g. see Talk desktop or clients etc).

@susnux
Copy link
Contributor

susnux commented Jun 19, 2025

BTW we now also have a OCS api for this in 32 and 31: #53569

@nickvergessen
Copy link
Member

BTW we now also have a OCS api for this in 32 and 31: #53569

Not sure this is sufficient from our side, as it would add another API request when people load the talk dashboard?

@susnux
Copy link
Contributor

susnux commented Jun 20, 2025

Not sure this is sufficient from our side, as it would add another API request when people load the talk dashboard?

Well on webui there is the js config (also as initalstate) and clients need to fetch it anyways.

But I still think everything or at least most what is provided as js config should be a capability.

@nextcloud-bot nextcloud-bot mentioned this pull request Aug 22, 2025
@Antreesy Antreesy marked this pull request as draft August 22, 2025 17:58
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
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 backport-request bug feature: caldav Related to CalDAV internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants