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

Introduce new notification for updates #488

Merged
merged 6 commits into from Mar 8, 2019

Conversation

Projects
None yet
3 participants
@xh3n1
Copy link
Member

xh3n1 commented Feb 28, 2019

Description

Done

  • Introduce new notification for update,
  • Remove "try automatic updater beta"
  • Check for update every 3 days
  • Save check time on the first install
  • Do not show notification on Dev
  • Hide "update now" for disabled automatic updater.
  • Fix tests

Related Issue

https://mantis.phplist.org/view.php?id=19760

TODO

  • Decide if the notification should be displayed for RCs too.
  • Remove old notification and checks.

xh3n1 added some commits Feb 28, 2019

Introduce new notification for updates
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Add checkupdate time on first install and check if its a dev version
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>

@xh3n1 xh3n1 force-pushed the update-notification-limit-pings branch from d1a1404 to 7f6ce05 Mar 1, 2019

@xh3n1 xh3n1 marked this pull request as ready for review Mar 1, 2019

@xh3n1 xh3n1 requested review from samtuke and suelaP Mar 1, 2019

@xh3n1

This comment has been minimized.

Copy link
Member Author

xh3n1 commented Mar 1, 2019

@samtuke cc: @suelaP Please let me know if the notification should be displayed for RCs too.

@samtuke

This comment has been minimized.

Copy link
Contributor

samtuke commented Mar 1, 2019

@xh3n1 Thanks. @suelaP : over to you.

xh3n1 added some commits Mar 4, 2019

Use %a format for comparing days
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
remove old notification
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@suelaP

This comment has been minimized.

Copy link
Member

suelaP commented Mar 4, 2019

I think this should be displayed for RCs too as whoever wants to test might want to see that. Other people might just ignore it. Although, I am not sure how good that will work with the check for updates every 3 days. @samtuke

@samtuke

This comment has been minimized.

Copy link
Contributor

samtuke commented Mar 4, 2019

@suelaP For hosting services and consultants who provide phpList as a service, encouraging their users to upgrade to an unstable RC would be undesirable. As a compromise I propose having two config file settings:

  • disable auto updater (disables all notifications)
  • disable rc updates (shows only stable updates)

Both would be enabled by default. A possible risk is that most admins who want to prevent RC notifications would take the more extreme approach of disabling the updater completely, which would undermine the benefit of having two options.

Thoughts?

@suelaP

This comment has been minimized.

Copy link
Member

suelaP commented Mar 4, 2019

I think providing options on both is the best approach, however I find the "disabled by default" for both quite radical.

I think we can leave it disabled for RC notifications and enabled for regular updates.
@samtuke

@samtuke

This comment has been minimized.

Copy link
Contributor

samtuke commented Mar 4, 2019

@suelaP Sorry, the double negatives introduced confusion -- I meant that both the config options could be set such that notifications for both RCs and stable updates would be shown by default in the interface; that was the suggestion.

Add config setting for RC candidates
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>

@xh3n1 xh3n1 force-pushed the update-notification-limit-pings branch from 53057d4 to 697e95e Mar 5, 2019

@xh3n1 xh3n1 removed the work in progress label Mar 6, 2019

@samtuke

This comment has been minimized.

Copy link
Contributor

samtuke commented Mar 7, 2019

Rerunning Travis tests to try and get to green

Change category
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>

@samtuke samtuke merged commit 6fd9ad5 into master Mar 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.