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

feat(settings): migrate AppAPI ExApps management to settings #48665

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

feat(settings): migrate AppAPI ExApps management to settings #48665

wants to merge 12 commits into from

Conversation

andrey18106
Copy link
Contributor

@andrey18106 andrey18106 commented Oct 11, 2024 •
edited
Loading

Summary

This PR migrates the UI for ExApp management to default Apps management page (settings app).
On backend side in settings app there is only addition for initial state for ExApps UI.
All ExApp related logic is in separate store that uses backend API part of AppAPI.
Also small fix for markdown render.

Screen Shot 2024-10-14 at 17 01 48

TODO

Checklist

@andrey18106
feat(app_api): add initial state data for AppAPI UI part
bcba911 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
feat(settings): migrate AppAPI ExApps management (1)
a67eefb 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: add Daemon badge in app sidebar (2)
f2898cb 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: correct markdown renderer
a290629 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: use global mutation, since app_api_apps is namespaced
7d73fc1 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: add missing state checks
1d700c1 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: minor fixes
90b4dc8 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106 andrey18106 mentioned this pull request Oct 11, 2024
Open
2 tasks
bigcat88
bigcat88 approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@bigcat88 bigcat88 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Checked, the functionality works.

Note for future reviewers: a backport of this pull request will be included in version 30.0.2, the merge is planned to be right after the release of 30.0.1, so that everyone has enough time to test.

@andrey18106
fix(ci): resolve eslint errors
4944dee 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
fix(ci): suppress UndefinedClass for AppAPI related classes since it'…
2068375 
…s bundled

Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
susnux
susnux requested changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Nothing very critical but a lot of stuff summing up. See comments.

apps/settings/lib/Controller/AppSettingsController.php Outdated
$defaultDaemonConfig = null;

if ($appApiEnabled) {
$exAppFetcher = Server::get(\OCA\AppAPI\Fetcher\ExAppFetcher::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably should be part of server somewhen^^

@provokateurin do you think we can have stubs here or will have to fill up the baseline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AppAPI is shipped since 30.0.1

apps/settings/lib/Controller/AppSettingsController.php Outdated Show resolved Hide resolved
apps/settings/src/components/AppList/AppItem.vue Outdated Show resolved Hide resolved
apps/settings/src/mixins/AppManagement.js Outdated Show resolved Hide resolved
apps/settings/src/mixins/AppManagement.js Outdated Show resolved Hide resolved
apps/settings/src/mixins/AppManagement.js Outdated Show resolved Hide resolved
apps/settings/src/mixins/AppManagement.js Outdated Show resolved Hide resolved
apps/settings/src/mixins/AppManagement.js Outdated Show resolved Hide resolved
apps/settings/src/store/app_api_apps.js Outdated
const mutations = {

APPS_API_FAILURE(state, error) {
showError(t('app_api', 'An error occurred during the request. Unable to proceed.') + '<br>' + error.error.response.data.data.message, { isHTML: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed, does the message contain USER information? Or is this just technical information?
If it has user information (not just technical), then rephrase the message as it will be likely truncated as the first part is even every long.

If it contains user facing information then just show that, as the first part is really unhelpful, otherwise just show the first part and log the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, it wasn't changed because it was originally clone from the settings "apps" Vuex store. Will remove.

apps/settings/src/store/app_api_apps.js Outdated Show resolved Hide resolved
@susnux susnux added this to the Nextcloud 31 milestone Oct 14, 2024
julien-nc
julien-nc reviewed Oct 14, 2024
View reviewed changes
Copy link
Member

@julien-nc julien-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'm not sure how likely it is to happen in a production environment but in my dev setup, some apps don't have a licence so when clicking on their list item, the sidebar does not open.
image

@susnux
Copy link
Contributor

susnux commented Oct 15, 2024

I'm not sure how likely it is to happen in a production environment but in my dev setup, some apps don't have a licence so when clicking on their list item, the sidebar does not open.

How can this happen? I was pretty sure the license is a required field in appinfo.xml thus the appstore should deny uploading such an app?

@andrey18106
Copy link
Contributor Author

I'm not sure how likely it is to happen in a production environment but in my dev setup, some apps don't have a licence so when clicking on their list item, the sidebar does not open.

How can this happen? I was pretty sure the license is a required field in appinfo.xml thus the appstore should deny uploading such an app?

It's a Test Deploy app, or any other manual-install type ExApp, it has no appstore information, only short info from DB.

@andrey18106
WIP: address review comments, small fixes
5cc9df8 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: migrate to Pinia, minor fixes
2689e5d 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
@andrey18106
WIP: remove app_api_apps vuex store
55f212e 
Signed-off-by: Andrey Borysenko <andrey18106x@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-nc julien-nc julien-nc left review comments

@susnux susnux susnux requested changes

@bigcat88 bigcat88 bigcat88 approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: apps management
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
4 participants
@andrey18106 @susnux @julien-nc @bigcat88