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

Reload the page when an app needs an update after being enabled #19526

Merged
merged 3 commits into from
Oct 6, 2015
Merged

Reload the page when an app needs an update after being enabled #19526

merged 3 commits into from
Oct 6, 2015

Conversation

nickvergessen
Copy link
Contributor

Fix #16251

@PVince81 @oparoz

Now displayes a notification and then reloads the page for you:

apps_-owncloud-_2015-10-02_12 02 32

@nickvergessen nickvergessen added this to the 8.2-current milestone Oct 2, 2015
@MorrisJobke
Copy link
Contributor

That is really nice :)

@karlitschek
Copy link
Contributor

good 👍

@MorrisJobke MorrisJobke self-assigned this Oct 2, 2015
@MorrisJobke
Copy link
Contributor

I would do this inline - right where error messages are also shown. Let me try it.

@MorrisJobke
Copy link
Contributor

I added the notification inline and added also a button to reload.

And I tested it with IE8 ;)

bildschirmfoto von 2015-10-02 14-09-25

@nickvergessen @owncloud/designers Does this look better? Or should I remove the button again? I just thought that it would be nice to see, that an action by the user is required.

@nickvergessen
Copy link
Contributor Author

I think it should not be green, another action is required, and anything else on the page will not work anymore.
That is why I used the global thing instead of the inline.

@MorrisJobke
Copy link
Contributor

I think it should not be green

yellow/orange?

@MorrisJobke
Copy link
Contributor

I think a notification is the wrong thing for this.

@MorrisJobke
Copy link
Contributor

Or we fade everything out (because it doesn't work anyway) and then print "app upgrade detected - will reload in 5 seconds"

@MorrisJobke
Copy link
Contributor

Something like:

bildschirmfoto am 2015-10-02 um 15 31 46

?

@MorrisJobke
Copy link
Contributor

@nickvergessen @owncloud/designers Next round :)

@MorrisJobke
Copy link
Contributor

And of course tested in IE8 and IE9 ;)

@nickvergessen
Copy link
Contributor Author

Can we reuse the modal from the remote sharing stuff?

@oparoz
Copy link
Contributor

oparoz commented Oct 2, 2015

I find the inline notification the best:

  • The admin can't miss it as he's just pressed the enable/update button
  • There is a button for easy reloading
  • It's not blocking the workflow

Now, if an update can take down the whole instance while it waits for the reload to happen, then I think the modal window has to be chosen. I would still add a button though. Who has time to wait 5 seconds? ;)

@nickvergessen
Copy link
Contributor Author

Now, if an update can take down the whole instance while it waits for the reload to happen,

This is the case, any pressing of another enable/disable button will not work.

@oparoz
Copy link
Contributor

oparoz commented Oct 2, 2015

This is the case, any pressing of another enable/disable button will not work.

OK for app installs, but can't we still navigate to other parts of ownCloud? I guess it would introduce other problems where an admin forgets to reload the page and then complains that something bad is happening.

@nickvergessen
Copy link
Contributor Author

Well any page refresh will bring up the update screen, so we are not breakinh anyything.
Just maaking sure the instance can be used again asap

@MorrisJobke
Copy link
Contributor

Can we reuse the modal from the remote sharing stuff?

Will do.

@MorrisJobke
Copy link
Contributor

I updated this to the OC.dialogs:

bildschirmfoto am 2015-10-05 um 08 48 10

@nickvergessen @oparoz Is this okay now?

@nickvergessen
Copy link
Contributor Author

Fine by me, I would use "Reload now" instead of "Ok", but that's nitpicking ;)

@MorrisJobke
Copy link
Contributor

Fine by me, I would use "Reload now" instead of "Ok", but that's nitpicking ;)

Not easy possible. Would require to redo the dialogs API 🙈

@MorrisJobke MorrisJobke force-pushed the issue-16251-reload-app-settings-when-app-needs-update branch from 129d031 to 54a0752 Compare October 5, 2015 08:54
DeepDiver1975
DeepDiver1975 reviewed Oct 5, 2015
View reviewed changes
settings/js/apps.js
showReloadMessage: function(appId) {
OC.dialogs.info(
t(
'files_sharing',
Copy link
Member

Choose a reason for hiding this comment

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

settings - not files_sharing

@MorrisJobke MorrisJobke force-pushed the issue-16251-reload-app-settings-when-app-needs-update branch from 54a0752 to f65697e Compare October 5, 2015 09:47
@MorrisJobke
Copy link
Contributor

@DeepDiver1975 Fixed

@oparoz
Copy link
Contributor

oparoz commented Oct 5, 2015

Would require to redo the dialogs API

DO IT! :)

@oparoz
Copy link
Contributor

oparoz commented Oct 5, 2015

  • What happens if I click on the x and not on "OK"
  • What happens if I press back in my browser

@MorrisJobke
Copy link
Contributor

DO IT! :)

Not two weeks before a release ;)

@MorrisJobke
Copy link
Contributor

What happens if I click on the x and not on "OK"

Then you can still do something (within the 5 seconds time window) - there is no way to handle the "close dialog" properly. And I don't want to introduce again more glue code. It should just provide a better way than the yellow notification.

What happens if I press back in my browser

You go back. Most likely it will reload the page and then you see the upgrade page ;)

@DeepDiver1975
Copy link
Member

DO IT! :)

As in throw the crap away and build something useful? Contributions in the scope of 9.0 are very much welcome. 😉

@DeepDiver1975 DeepDiver1975 force-pushed the issue-16251-reload-app-settings-when-app-needs-update branch from f65697e to 264d123 Compare October 5, 2015 20:07
@rullzer
Copy link
Contributor

rullzer commented Oct 6, 2015

Works.
Seems like the best we can get for 8.2.
👍

@MorrisJobke MorrisJobke removed their assignment Oct 6, 2015
@MorrisJobke
Copy link
Contributor

@nickvergessen @oparoz Can I ask you to do a final review?

@nickvergessen
Copy link
Contributor Author

Just tested it and it works okay.

However as an admin I wonder if the app has been enabled now or not.
That is not visible in the dialog, so I'd adjust that message to something like:
The app was enabled, but needs to be updated. You will be redirected to the update page in 5 seconds.

other then that 👍

@MorrisJobke
Copy link
Contributor

@nickvergessen Done.

DeepDiver1975 added a commit that referenced this pull request Oct 6, 2015
@DeepDiver1975
Merge pull request #19526 from owncloud/issue-16251-reload-app-settin…
6651aa1 
…gs-when-app-needs-update

Reload the page when an app needs an update after being enabled
@DeepDiver1975 DeepDiver1975 merged commit 6651aa1 into master Oct 6, 2015
@DeepDiver1975 DeepDiver1975 deleted the issue-16251-reload-app-settings-when-app-needs-update branch October 6, 2015 10:36
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
4 - To release settings:apps sev2-high
Projects
None yet
Milestone
8.2
Development

Successfully merging this pull request may close these issues.

App manager stops working when an app needs to upgrade oC
6 participants
@nickvergessen @MorrisJobke @karlitschek @oparoz @DeepDiver1975 @rullzer