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

Use NavigationManager instead of AppManager to handle custom apps order #47523

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Use NavigationManager instead of AppManager to handle custom apps order #47523

merged 7 commits into from
Sep 9, 2024

Conversation

provokateurin
Copy link
Member

Summary

The custom defaults "apps" and order were using the apps and not navigation entries (see the linked issue for more details).
The confusion lead to a few problems that can only be solved by only using the NavigationManager and not the AppManager.
Most of the logic is the same, except for checking against available navigation entries and not apps.

I tried touching as little as possible, all the frontend code is still the same and uses the incorrect "app" over "navigation entry" concept. On the frontend it still makes a bit sense, since the concept is a lot easier to grasp for users.
The config keys on the backend are kept the same to avoid migrations and deprecations.

In theory one could spend a lot more time on trying to fix this, but I already tried that a bit and it is a rabbit hole that is too deep for fixing and backporting.

The last commit is a fix that is useful even without all the other changes. In case an app is disabled while also being used for a custom order, the frontend would display "undefined" entries.

As for backporting, I kept the old broken interface by providing a proxy implementation. I hope this is good enough to be backported.

Checklist

@provokateurin provokateurin added this to the Nextcloud 31 milestone Aug 27, 2024
@provokateurin provokateurin requested review from susnux, a team, ArtificialOwl, artonge and come-nc and removed request for a team August 27, 2024 11:45
come-nc
come-nc reviewed Aug 27, 2024
View reviewed changes
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.

What happens if you actually set an external site as default entry, you cannot use Nextcloud anymore? (login would redirect to external site, no?)

lib/private/App/AppManager.php Outdated Show resolved Hide resolved
lib/private/NavigationManager.php Outdated Show resolved Hide resolved
lib/private/NavigationManager.php Outdated Show resolved Hide resolved
lib/private/NavigationManager.php Outdated Show resolved Hide resolved
lib/private/TemplateLayout.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor

come-nc commented Aug 27, 2024

Is the comment in

server/config/config.sample.php

Lines 1169 to 1178 in 558877c

/**
* Set the default app to open on login. Use the app names as they appear in the
* URL after clicking them in the Apps menu, such as documents, calendar, and
* gallery. You can use a comma-separated list of app names, so if the first
* app is not enabled for a user then Nextcloud will try the second one, and so
* on. If no enabled apps are found it defaults to the dashboard app.
*
* Defaults to ``dashboard,files``
*/
'defaultapp' => 'dashboard,files',
still true after your PR is merged?

@provokateurin
Copy link
Member Author

What happens if you actually set an external site as default entry, you cannot use Nextcloud anymore? (login would redirect to external site, no?)

It still works, even if the user doesn't have access because all invalid entries are filtered out (e.g. because the external site is only allowed for a group the user is not part of). I think this might actually be broken on master, since only my last commit actually does this check.

still true after your PR is merged?

No, it has to be updated. I'm not sure thought how the admin could effectively get the navigation entry id without poking the API 🤔

@provokateurin
Copy link
Member Author

Cypress failure is related for once 😅

@provokateurin
Copy link
Member Author

Cypress failure was because of a deeper issue in the backend side which I also fixed.

@provokateurin provokateurin force-pushed the fix/theming/custom-apps-order branch 2 times, most recently from 5d64938 to 4dafd92 Compare September 5, 2024 08:37
@provokateurin
Copy link
Member Author

Fix will not be backported to any version due to the new interfaces and the close release of 30.

@provokateurin
feat(NavigationManager): Add default entries handling
b0baaae 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
refactor(AppManager): Deprecated default apps handling
70ed08d 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
fix(URLGenerator): Use NavigationManager to generate default page URL
01ec676 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
fix(TemplateLayout): Use NavigationManager to get default app name
6696411 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
fix(theming): Use NavigationManager to handle default entries
0a3093d 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
fix(NavigationManager): Skip invalid default navigation entries
d5e98cd 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
Copy link
Member Author

Cypress issues are resolved (caused by bugs in the backend). @susnux can you review again?

@provokateurin
Copy link
Member Author

/compile amend /

@provokateurin @nextcloud-command
docs(config): Explain how to retrieve navigation entry IDs for 'defau…
c0ac279 
…ltapp' option

Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@provokateurin provokateurin merged commit 3a85997 into master Sep 9, 2024
174 checks passed
@provokateurin provokateurin deleted the fix/theming/custom-apps-order branch September 9, 2024 16:04
@provokateurin provokateurin mentioned this pull request Sep 22, 2024
Closed
@provokateurin provokateurin added the pending documentation This pull request needs an associated documentation update label Sep 29, 2024
@nickvergessen
Copy link
Member

nickvergessen commented Sep 30, 2024 •
edited
Loading

This actually broke the guests app, which is going nasty stuff:
https://github.com/nextcloud/guests/blob/master/lib/FilteredNavigationManager.php

Can you fix it or raise a ticket there?

@nickvergessen nickvergessen mentioned this pull request Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@come-nc come-nc come-nc approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@artonge artonge Awaiting requested review from artonge artonge was automatically assigned from nextcloud/server-backend

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug feature: theming pending documentation This pull request needs an associated documentation update
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.

[Bug]: "Could not set global default apps"
4 participants
@provokateurin @come-nc @nickvergessen @susnux