Display app names in update page for app updates #17434

PVince81 · 2015-07-07T10:15:05Z

Whenever the update page is displayed for apps, show app names instead
of the core update text.

Fixes #16272

TODO: add unit test
To discuss: should we move getIncompatibleApps and getAppsNeedingUpgrade to the IAppManager interface ? I'd rather keep them private but "apps.php" is horribly static... @icewind1991 @MorrisJobke @nickvergessen @oparoz what do you think ?

PVince81 · 2015-07-07T10:15:47Z

TEST: core update: shows core update page
TEST: app update alone: shows app update page
TEST: core + app update: shows core update page

PVince81 · 2015-07-07T10:16:00Z

TEST: auto-disable third party apps on core update still works

PVince81 · 2015-07-07T10:41:11Z

Setting to "To review" because I'd like to get comments 😄

PVince81 · 2015-07-07T12:11:43Z

Fixes #17437 too, @jancborchardt

PVince81 · 2015-07-07T12:55:14Z

@jancborchardt I'll revert and fix separately, it's a different piece of code.

PVince81 · 2015-08-19T12:41:12Z

Ok looks like nobody objected the last 1.5 months so I'll keep these methods static.

Time for unit tests then.

PVince81 · 2015-08-19T12:44:36Z

Basically it looks like this:

@jancborchardt

oparoz · 2015-08-19T13:14:59Z

Regarding the static methods. I don't think app devs would have a use for it, unless they write an app manager?

Regarding the design, I think it would be nice if apps could be shown using bullet points. It makes it clearer what is info-text and what is an app.

PVince81 · 2015-08-19T13:17:31Z

I checked the code and remembered why I can't move them easily to the app manager: the app manager is missing methods like "isAppCompatible" and "shouldUpgrade". I don't think we want these on the app manager either.

For the bullet points I'll let @jancborchardt decide.
I think until now we never had bullet points on this page for lists. Another list is the one that shows you what apps are getting disabled during a core update.

oparoz · 2015-08-19T13:24:02Z

Another list is the one that shows you what apps are getting disabled during a core update.

Ah, yes, that needs fixing as well because it only shows the folder name iirc.

PVince81 · 2015-08-19T13:25:19Z

@oparoz please raise another ticket for that. Thanks.

Whenever the update page is displayed for apps, show app names instead of the core update text.

PVince81 · 2015-08-19T16:05:49Z

Done.

I moved getIncompatibleApps() and getAppsNeedingUpgrade() to the AppManager class.
I also added AppManager->getAppInfo() that calls the old API to make it mockable for unit tests.

Please review @icewind1991 @nickvergessen @MorrisJobke @Xenopathic @oparoz

ghost · 2015-08-19T17:41:42Z

🚀 Test PASSed.🚀

RobinMcCorkell · 2015-08-19T18:34:05Z

core/templates/update.admin.php

@@ -1,7 +1,20 @@
 <div class="update" data-productname="<?php p($_['productName']) ?>" data-version="<?php p($_['version']) ?>">
 	<div class="updateOverview">
+		<?php if ($_['isAppsOnlyUpgrade']) { ?>


Do app upgrades not occur at the same time as core upgrades? If they do, then we should surely display 'The following apps will be updated' alongside 'ownCloud will be updated'?

As far as I can tell (without doing an actual upgrade), we will see:

ownCloud will be updated to version x

External file storage (files_external)
My awesome app (xenopathic_is_awesome)

IMHO we still need to display 'The following apps will be updated' under 'ownCloud will be updated'

Yeah, I also thought about this.
I'll try and add them to both pages.

PVince81 · 2015-08-20T08:09:08Z

TODO: add list of apps to upgrade on both core and app upgrade modes

PVince81 · 2015-08-20T08:10:04Z

TODO: keep warning about backups in both modes

jancborchardt · 2015-08-20T08:48:43Z

I agree with @oparoz that it would be nicer to use a list to show the apps. That way they are easily readable and also not center-aligned.

PVince81 · 2015-08-20T08:51:14Z

It is already a ul/li list, so I'll switch its CSS style to use bullets instead.
I assume we'll want this for the list of disabled apps too.

TODO: add bullets to CSS for "ul" in update page

Apps to update and to disable will always be shown. Main title changes only when apps need updated, not core. Added bullet style. Exclude incompatible apps from updated apps list.

scrutinizer-notifier · 2015-08-20T09:16:09Z

A new inspection was created.

PVince81 · 2015-08-20T09:17:47Z

Done.
I also changed the wording a bit:

Note that the main title changes if a core update is required too.

Also fixed:

BUG: if an app needing update is also incompatible, don't show it in the update list

RobinMcCorkell · 2015-08-20T09:30:40Z

I'm not 100% sure on the 'Apps update required' wording, but I can't think of any alternatives, and the code looks good 👍

MorrisJobke · 2015-08-20T09:35:02Z

disabled 3rdparty apps aren't listed

PVince81 · 2015-08-20T09:39:49Z

@MorrisJobke they are listed after the update. Something to be done separately because it might require changing things in the update order.

PVince81 · 2015-08-20T09:40:33Z

Note: the original purpose of this PR is to solve the confusion that people have between core update and app update ("why does my ownCloud suddenly wants to update again ?")

MorrisJobke · 2015-08-20T09:41:43Z

Then it is working just fine 👍

ghost · 2015-08-20T09:49:21Z

🚀 Test PASSed.🚀

Display app names in update page for app updates

jancborchardt · 2015-08-20T10:57:20Z

core/templates/update.admin.php

@@ -1,12 +1,26 @@
 <div class="update" data-productname="<?php p($_['productName']) ?>" data-version="<?php p($_['version']) ?>">
 	<div class="updateOverview">
+		<?php if ($_['isAppsOnlyUpgrade']) { ?>
+		<h2 class="title bold"><?php p($l->t('Apps update required.')); ?></h2>


This should say »App update required« (App without s, no dot at the end.)

Pull request at #18453

PVince81 mentioned this pull request Jul 7, 2015

Don't mention the oC version number when upgrading apps #16272

Closed

PVince81 added the 3 - To Review label Jul 7, 2015

PVince81 added this to the 8.2-next milestone Jul 7, 2015

PVince81 self-assigned this Jul 7, 2015

PVince81 mentioned this pull request Jul 7, 2015

Update screen layout and wording fix #17437

Closed

PVince81 force-pushed the update-showappnameonappupdate branch from 0c3c984 to e2d100e Compare July 7, 2015 12:59

PVince81 mentioned this pull request Aug 12, 2015

8.1.1 Enabling an app always triggers core update #18235

Closed

Display app names in update page for app updates

b919ae9

Whenever the update page is displayed for apps, show app names instead of the core update text.

PVince81 force-pushed the update-showappnameonappupdate branch from e2d100e to b919ae9 Compare August 19, 2015 16:04

RobinMcCorkell reviewed Aug 19, 2015
View reviewed changes

Additions to update page

a2674b2

Apps to update and to disable will always be shown. Main title changes only when apps need updated, not core. Added bullet style. Exclude incompatible apps from updated apps list.

MorrisJobke added 4 - To release and removed 3 - To Review labels Aug 20, 2015

MorrisJobke added a commit that referenced this pull request Aug 20, 2015

Merge pull request #17434 from owncloud/update-showappnameonappupdate

06d8edd

Display app names in update page for app updates

MorrisJobke merged commit 06d8edd into master Aug 20, 2015

MorrisJobke deleted the update-showappnameonappupdate branch August 20, 2015 09:50

jancborchardt reviewed Aug 20, 2015
View reviewed changes

PVince81 mentioned this pull request Aug 21, 2015

Upgrade Status Report #14680

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display app names in update page for app updates #17434

Display app names in update page for app updates #17434

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Aug 19, 2015

PVince81 commented Aug 19, 2015

oparoz commented Aug 19, 2015

PVince81 commented Aug 19, 2015

oparoz commented Aug 19, 2015

PVince81 commented Aug 19, 2015

PVince81 commented Aug 19, 2015

ghost commented Aug 19, 2015

RobinMcCorkell Aug 19, 2015

RobinMcCorkell Aug 19, 2015

PVince81 Aug 20, 2015

PVince81 commented Aug 20, 2015

PVince81 commented Aug 20, 2015

jancborchardt commented Aug 20, 2015

PVince81 commented Aug 20, 2015

scrutinizer-notifier commented Aug 20, 2015

PVince81 commented Aug 20, 2015

RobinMcCorkell commented Aug 20, 2015

MorrisJobke commented Aug 20, 2015

PVince81 commented Aug 20, 2015

PVince81 commented Aug 20, 2015

MorrisJobke commented Aug 20, 2015

ghost commented Aug 20, 2015

jancborchardt Aug 20, 2015

MorrisJobke Aug 20, 2015

jancborchardt Aug 20, 2015

Display app names in update page for app updates #17434

Display app names in update page for app updates #17434

Conversation

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

PVince81 commented Aug 19, 2015

PVince81 commented Aug 19, 2015

oparoz commented Aug 19, 2015

PVince81 commented Aug 19, 2015

oparoz commented Aug 19, 2015

PVince81 commented Aug 19, 2015

PVince81 commented Aug 19, 2015

ghost commented Aug 19, 2015

RobinMcCorkell Aug 19, 2015

Choose a reason for hiding this comment

RobinMcCorkell Aug 19, 2015

Choose a reason for hiding this comment

PVince81 Aug 20, 2015

Choose a reason for hiding this comment

PVince81 commented Aug 20, 2015

PVince81 commented Aug 20, 2015

jancborchardt commented Aug 20, 2015

PVince81 commented Aug 20, 2015

scrutinizer-notifier commented Aug 20, 2015

PVince81 commented Aug 20, 2015

RobinMcCorkell commented Aug 20, 2015

MorrisJobke commented Aug 20, 2015

PVince81 commented Aug 20, 2015

PVince81 commented Aug 20, 2015

MorrisJobke commented Aug 20, 2015

ghost commented Aug 20, 2015

jancborchardt Aug 20, 2015

Choose a reason for hiding this comment

MorrisJobke Aug 20, 2015

Choose a reason for hiding this comment

jancborchardt Aug 20, 2015

Choose a reason for hiding this comment