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

Add update all button to the app list #17186

Closed
wants to merge 6 commits into from

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Sep 17, 2019

Fix #6989

@GretaD GretaD force-pushed the feature/6989/update_all_apps_button branch from e71e0fa to 84f3b41 Compare September 17, 2019 20:55
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

This is going to make managing an install much easier. Thank you.

return this.apps.find(app => app.update)
},
showUpdateAll(){
return this.hasPendingUpdate && ['installed', 'updates'].indexOf(this.category) !== -1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return this.hasPendingUpdate && ['installed', 'updates'].indexOf(this.category) !== -1
return this.hasPendingUpdate && ['installed', 'updates'].includes(this.category)

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Great work :)
See others comments ;)

@kesselb
Copy link
Contributor

kesselb commented Sep 19, 2019

image

@kesselb
Copy link
Contributor

kesselb commented Sep 19, 2019

  1. The "Update all" button is only visible on the "your apps" page.

  2. Add $appData['update'] = '19.0.1'; above to show the update button for every app and then press the update all button. The result is a password confirmation box for every app with an update. forEach is blocking but the dispatch method not. I don't think we can wait for a event emitted by dispatch so the requiredAdmin call needs to be done before the emit to show only one password confirmation box.

  3. Current implementation will update the apps at the same time. I'm not sure if this is a good idea. A new "updateAll" route would be more stable.

@skjnldsv
Copy link
Member

2. I don't think we can wait for a event emitted by dispatch so the requiredAdmin call needs to be done before the emit to show only one password confirmation box.

Yes, we can :)
https://github.com/nextcloud/contacts/blob/380cda3c6a43f7a49ea8cd31869ba276c458c702/src/components/Settings/SettingsAddressbook.vue#L186

@GretaD

@GretaD
Copy link
Contributor Author

GretaD commented Sep 24, 2019

  1. The "Update all" button is only visible on the "your apps" page.

You propose to be also in other pages? Currently is at "your apps" and "Update" page.

@kesselb
Copy link
Contributor

kesselb commented Sep 24, 2019

You propose to be also in other pages? Currently is at "your apps" and "Update" page.

I did but changed my mind now 🤣 I guess if we add the update all button to the disabled apps page people would expect that only disabled apps are updated and the same for active apps. But this is to complex so your apps and updates is fine.

@georgehrke
Copy link
Member

The result is a password confirmation box for every app with an update. forEach is blocking but the dispatch method not. I don't think we can wait for a event emitted by dispatch so the requiredAdmin call needs to be done before the emit to show only one password confirmation box.

2. I don't think we can wait for a event emitted by dispatch so the requiredAdmin call needs to be done before the emit to show only one password confirmation box.

Yes, we can :)
https://github.com/nextcloud/contacts/blob/380cda3c6a43f7a49ea8cd31869ba276c458c702/src/components/Settings/SettingsAddressbook.vue#L186

@skjnldsv But you are inside an forEach loop. So even if the callback inside the foreach loop is returning a promise, it will happily ignore that and just call the next callback.

Leaving us with two options:

  1. Solve this problem in javascript:
  1. Solve this problem in php:
  • Just add a dedicated route that updates all apps with one request.

I would personally prefer 1.

@skjnldsv @kesselb ^

@kesselb
Copy link
Contributor

kesselb commented Oct 7, 2019

@GretaD could you rebase? ;)

@GretaD
Copy link
Contributor Author

GretaD commented Oct 7, 2019

@GretaD could you rebase? ;)

darn, i thought i already did

@GretaD GretaD force-pushed the feature/6989/update_all_apps_button branch from 983f7e5 to 9de7f10 Compare October 7, 2019 14:49
@skjnldsv
Copy link
Member

skjnldsv commented Oct 8, 2019

darn, i thought i already did

Because lots of changes went into server again :)

/settings is now in /apps/settings

Signed-off-by: Greta Doci <gretadoci@gmail.com>
Signed-off-by: Greta Doci <gretadoci@gmail.com>
Signed-off-by: Greta Doci <gretadoci@gmail.com>
Signed-off-by: Greta Doci <gretadoci@gmail.com>
Signed-off-by: Greta Doci <gretadoci@gmail.com>
Signed-off-by: Greta Doci <gretadoci@gmail.com>
@GretaD GretaD force-pushed the feature/6989/update_all_apps_button branch from 9de7f10 to a27b0fb Compare October 8, 2019 11:31
@GretaD GretaD mentioned this pull request Oct 11, 2019
@ChristophWurst ChristophWurst deleted the feature/6989/update_all_apps_button branch December 3, 2019 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add update all button in the apps management
6 participants