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

Update notifications for core and apps #24444

Merged
merged 8 commits into from
May 25, 2016

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen commented May 4, 2016

  • Trigger notifications for new updates in a background job
  • Parse the notifications
  • Make notification user group configurable in the UI
  • Automatically delete the notification when an app/core was updated => No hooks, needs to move to "on rendering"
  • Add unit tests ?!

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @BernhardPosselt, @DeepDiver1975 and @LukasReschke to be potential reviewers

@nickvergessen
Copy link
Contributor Author

Done, ready for review

@nickvergessen nickvergessen force-pushed the update-notifications-for-core-and-apps branch from 39b4e3b to dbf0a10 Compare May 10, 2016 09:31
@nickvergessen
Copy link
Contributor Author

if (newChannel === 'git' || newChannel === 'daily') {
$('#oca_updatenotification_groups em').removeClass('hidden');
} else {
$('#oca_updatenotification_groups em').addClass('hidden');
Copy link
Contributor

Choose a reason for hiding this comment

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

protip: $(...).toggleClass('hidden', boolValue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't toggle, because then I need to catch the case when you change from stable to product and not toggle...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same for beta, and then it's messier than now

Copy link
Contributor

@PVince81 PVince81 May 10, 2016

Choose a reason for hiding this comment

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

Not really, this code should behave the same as yours, but shorter:
$('#oca_updatenotification_groups em').toggleClass('hidden', !(newChannel === 'git' || newChannel === 'daily'))

Anyway, it's not a biggie.

@PVince81
Copy link
Contributor

Will the notification be recreated during the next update check or is there code that makes sure the notification only appears once ? I didn't seem to find it, maybe it's a feature of the notifications app ?

@nickvergessen
Copy link
Contributor Author

Yes, each new update will generate a new notification, but before that we delete the old notifications, in case one had them still unread.
Also the app removes the notifications, when the update has already been installed, so the other admins don't need to click the notification away

return $this->users;
}

$notifyGroups = json_decode($this->config->getAppValue('updatenotification', 'notify_groups', '["admin"]'), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

if no groups were selected, disable the background job completely (or make it no-op)
In some scenarios an admin might not want to have a bkg job pinging the app store for every app regularily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, maybe one can just disable the update notification app

@PVince81
Copy link
Contributor

App update notification doesn't seem to work for me.
What I did:

  1. Install the Notes app from Rekkyt from the app store
  2. Edit apps/notes/info.xml and set the version to 2.0.0 (it was 2.0.1)
  3. update oc_jobs set last_run=null;
  4. run CLI cron
  5. Refresh the web page

There are no new entries in oc_notifications.
If I go to the apps page, the yellow notification appears.

The core update notification works though, so the jobs does run but something might not work correctly with apps updates.

@PVince81
Copy link
Contributor

Just debugged a bit, and I saw it go here: https://github.com/owncloud/core/pull/24444/files#diff-b720db44b64d429650b1fa2d2bbbc777R129 even though there is no entry in oc_notifications (was never for this app)

Ok, so I tried doing this: update oc_appconfig set configvalue="2.0.0" where configkey="notes" and appid="updatenotification";.

Now when I run cron, there's an entry in oc_notifications.

MariaDB [owncloud]> select * from oc_notifications;
+-----------------+--------------------+-------+------------+-------------+-----------+------------------+--------------------+---------+--------------------+----------------------------------------------------+---------+
| notification_id | app                | user  | timestamp  | object_type | object_id | subject          | subject_parameters | message | message_parameters | link                                               | actions |
+-----------------+--------------------+-------+------------+-------------+-----------+------------------+--------------------+---------+--------------------+----------------------------------------------------+---------+
|               3 | updatenotification | admin | 1462890969 | notes       | 2.0.1     | update_available | []                 |         | []                 | http://localhost/index.php/settings/apps#app-notes | []      |
+-----------------+--------------------+-------+------------+-------------+-----------+------------------+--------------------+---------+--------------------+----------------------------------------------------+---------+

However when I refresh the web UI, that entry just disappears and nothing appears in the UI !

@nickvergessen
Copy link
Contributor Author

I guess because your cheat was incomplete and the installed_version in appconfig is 2.0.1?

@PVince81
Copy link
Contributor

Indeed. Works now 👍

@nickvergessen
Copy link
Contributor Author

Second review @schiesbn @ChristophWurst @rullzer

@rullzer
Copy link
Contributor

rullzer commented May 24, 2016

👍

@PVince81
Copy link
Contributor

@nickvergessen mind rebasing so we can see at least the basic DB tests pass ? Thanks (results timed out)

@nickvergessen
Copy link
Contributor Author

I just ran them locally and they all passed, but sure can slap jenkins in a bit

@PVince81
Copy link
Contributor

@nickvergessen here, use this 👢

@nickvergessen nickvergessen force-pushed the update-notifications-for-core-and-apps branch from dbf0a10 to a1e872a Compare May 24, 2016 09:27
@nickvergessen
Copy link
Contributor Author

Done

@PVince81 PVince81 merged commit c36cf30 into master May 25, 2016
@PVince81 PVince81 deleted the update-notifications-for-core-and-apps branch May 25, 2016 07:13
@PVince81
Copy link
Contributor

CC @carlaschroder @jospoortvliet for this enhancement

@nickvergessen
Copy link
Contributor Author

@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants