Skip to content

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Sep 11, 2025

Summary

Currently, enabled apps are returned, whether they are installed or not. This is a behavioral breaking change compared to 29.

Based this now on 30 for nextcloud/univention-app#208, and I am not sure this is what we really want in server (cf. my comment at https://github.com/nextcloud/server/pull/49648/files#r2341962849).

If so, I'd do a new PR based on master, which has diverges further.

Checklist

@blizzz blizzz requested a review from come-nc September 11, 2025 18:16
@blizzz blizzz added bug 2. developing Work in progress labels Sep 11, 2025
@blizzz
Copy link
Member Author

blizzz commented Sep 12, 2025

otoh not sure whether this approach might cause other side effects, as getInstalledAppsValues worked like this in 29 already. I guess I am going to change it to apply it on install command level again, but this will involve legacy API again, so even less acceptable for inclusion. Alternatively extending API with returning all installed apps with their paths… would be separate effort aimed at master then.

UPDATE:
For reference, previous, now dropped, commits I talked about were:

- AppManager::isInstalled() is misleading, as it checks only whether it is
  enabled. But an app might not be present in some edge cases.
- AppManager::getAppPath() does however only check whether an app dir is
  present, independent of the enabled-state.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/return-actually-installed-apps-stable30 branch from 8e06240 to ac6653e Compare September 12, 2025 09:48
@blizzz blizzz changed the title [stable30] fix(AppManager): getInstalledAppValues shall return actually installed apps [stable30] fix(Apps): fix install command check on existing apps Sep 12, 2025
blizzz added a commit to nextcloud/univention-app that referenced this pull request Sep 12, 2025
- includes nextcloud/server#55065

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz mentioned this pull request Sep 12, 2025
6 tasks
blizzz added a commit to nextcloud/univention-app that referenced this pull request Sep 12, 2025
- includes nextcloud/server#55065

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
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 this is not a good change.

To install an application manually, you put it in the apps folder and then run app:install, no? With this change it would not be possible anymore?

@blizzz
Copy link
Member Author

blizzz commented Sep 15, 2025

To install an application manually, you put it in the apps folder and then run app:install, no? With this change it would not be possible anymore?

No, you run occ:enable. Which installs it from appstore, if it is not present.

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 29, 2025
@blizzz blizzz requested review from a team, ArtificialOwl, CarlSchwan and provokateurin and removed request for a team September 29, 2025 07:53
@blizzz blizzz disabled auto-merge September 29, 2025 08:35
@blizzz blizzz merged commit 5ad84c1 into stable30 Sep 29, 2025
183 of 189 checks passed
@blizzz blizzz deleted the fix/noid/return-actually-installed-apps-stable30 branch September 29, 2025 08:35
@blizzz blizzz mentioned this pull request Oct 14, 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 bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants