@bx80 opened this Pull Request on September 27th 2021 Contributor

Description:

Fixes #16805

Adds a "What's new" notification icon to the admin top menu (new material icon added to the matomo font)
Defines a new event CoreAdminHome.getWhatIsNew which plugins can register for and use to provide an array of items representing their significant changes. These will then be displayed on the "What's new" popup.

This approach leaves individual plugins responsible for updating or removing their "What's new" entries. Perhaps there should be some sort of expiry option? If a plugin adds a "What's new" entry but then isn't updated for 5 years then we wouldn't really want that 5 year old "new" entry to still be shown.

Review

@bx80 commented on September 28th 2021 Contributor

After discussions with @tsteur and input from @sgiehl, I plan to make the following changes:

  • Load the new features/changes from a JSON file in each plugin rather than have plugins pass changes via events.
    • When any update or plugin activation/deactivation happens then check each plugin for an optional changes.json file and merge them all together into an single combined changes.json file.
    • Have each change require a date in additional to the name, description and link, so that changes can be shown in date order.
    • The "What's new" overlay should then be populated from the combined changes.json file on demand.
    • For expiry of old changes, implement a rolling window where a minimum quantity of the most recent (20?) updates are always shown no matter how old they are, after which any updates older than (180?) days are not shown.
  • Store the date that each user last viewed the "What's new" overlay.
  • Change the menu bar icon to material icon "bell" same as cloud (the current icon-new_releases looks too similar to help).
  • Show an indicator on the "What's new" menu icon if the date of the combined changes.json file is newer than the date the user last viewed "What's new" (ie. there are changes they haven't seen yet). Using material icon "bell ringing" as the indicator in place of the normal "bell" icon.
  • Add a UI test for the "What's new" overlay (will need to mock in a consistent set of changes).
@sgiehl commented on September 30th 2021 Member

@bx80 haven't had a closer look yet. But there might be a disadvantage with this implementation. If the changes are sorted by the date defined in the json file, that wouldn't reflect when those changes got available in the actual Matomo instance. If you for example install/update a plugin that was already available for quite a while the changes might not even be visible when there are newer changes in other components, right?

@bx80 commented on September 30th 2021 Contributor

If the changes in a newly installed plugin were older than the 180 day cut off then, yes they might not be shown, if they are within 180 days then they will always be shown no matter how many other changes there are (might have to scroll though!) I'm not sure there is a perfect solution here without tracking which user has seen which individual change, which is probably an overkill.

We could start tracking the plugin install/update date and then also show the last x changes for plugins that were installed or updated in the last 30 days no matter how old those changes are, they'd appear at the bottom of the list if the dates were a while ago, but they at least be there. There would need to be further checks for fresh installs to prevent every plugin getting on the list because they were all just installed. Maybe the installed/updated date would need to be more recent that the Matomo setup date. This adds a fair bit of complexity,

I don't think we could use solely the plugin install/update date to determine the change list order as eventually a single plugin could have hundreds of changes defined going back years and we wouldn't want all of them to be top of the list just because there was a minor update last week.

@github-actions[bot] commented on October 14th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@tsteur commented on October 15th 2021 Member

@bx80 are you still working on this issue? I haven't been reviewing as I vaguely remember we talked about potentially somehow slightly changing it to mark at what time the what's new was seen and then potentially remember that. If there is an efficient solution for it without storing heaps of information. Or maybe we just need to push all "what's new" entries into a table after all? Meaning eg when installing a plugin we look for new "what's new" entries and push them into the database and show them that way in the right order or something?

I'll remove the "needs review" label for now. Feel free to add it again if it's ready to review.

@bx80 commented on November 4th 2021 Contributor

@tsteur Just pickling this up again. The current implementation of this issue gets all the changes.json items for all plugins, sorts them by the date of the change and hides any items older than six months, always leaving at least x items no matter how old. Whenever a user views the changes list we store the last viewed date. If there are changes dated newer than the last time the list was viewed then the menu icon is changed to a ringing bell, otherwise it's a non-ringing bell and they can still see any recent changes based on change date.

As pointed out by @sgiehl automatically hiding items by change date causes a problem if for example, a plugin is installed 7 months after it's last release then the 'new' changes won't be shown at all.

We could stop hiding change items on based date and only hide/remove them when the user has viewed them but then this causes other problems - on a new install there could be hundreds unread of changes across all plugins. Also currently we're just counting the user opening the changes list dialog as having viewed the items - there is the possibility that they just clicked the changes list by mistake, close it and now the all change items have been removed without actually being read.

How about the following approach:

  • Don't hide any changes based on the change date.
  • Have two tabs on the dialog, 'New' and 'Read'(?), the dialog always opens to the 'New' tab showing a list of unread changes sorted by change date.
  • Each change item that hasn't been read has a 'dismiss'/'ok' button which will mark it as read and move it to the 'Read' list for that user. (each change item's read status to be stored somewhere in user settings or a new table if necessary)
  • Have a dismiss-all/mark-all-as-read button which will mark all unread 'New' changes as 'Read'
  • If there are any unread change items then the menu icon will be a ringing bell and open to the 'New' list, otherwise it will be a non-ringing bell and open to the 'Read' tab.
  • Documentation for plugin authors to recommend that old, obsolete changes be pruned from their changes.json when providing an update, so we don't end up with an entire plugin version history appearing in the changes list when a plugin is newly installed.

Variant B: As above but no dismiss buttons, just opening the dialog marks all 'New' changes items as 'Read', less clicking and they can always see the items on the 'Read' list later on.

Variant C: Don't have an unread tab at all, dismissed/viewed items are permanently hidden. Easier to implement. If there are no new items then we show a 'nothing new message' or hide the icon entirely. Probably not good to combine with B

What do you think?

@tsteur commented on November 5th 2021 Member

I'm getting bit afraid it might be getting too complicated. I just checked some changelog/whatsnew services and there are usually not really two different tabs.

What about something similar to our update logic.

We could a directory say whatsnew and files for versions in there. Say

plugins/QueuedTracking/whatsnew/4.0.5.json
plugins/QueuedTracking/whatsnew/4.0.6.json
plugins/QueuedTracking/whatsnew/4.0.10.json
// and in core maybe
core/whatsnew/4.0.4.json

or even simpler might be to have all in one file like plugins/QueuedTracking/whatsnew.json and indexed by version number or have a key version number similar to your example had a date in there. Or of course it could come from an event etc.

Core knows which core/plugin version we are on, and which version are updating to. Then we could look for files in "whatsnew" directory similar to https://github.com/matomo-org/matomo/blob/4.x-dev/core/Updater.php#L343-L390 when someone is updating and push each change into a database table. That way we know which are the newest entries and which ones not.

a table like below

id | time | plugin_name | version | message

The problem might be finding the right place to check for these without trying to write these values to the DB too often. It might work in the same method where we also look for updates.

Would something like this be maybe a bit simpler/quicker to implement?

The benefit would be simpler UI, not needing to store what which user has seen, or read etc.

@bx80 commented on November 5th 2021 Contributor

@tsteur That's fairly similar to what we're doing now, just really a case of replacing the date in whatsnew.json with a version number and pushing records into a new table. Might be cleaner to just read from a single file rather than having to check for multiple files by version name if the update jumps several versions.

From a UI perspective we'd still have a growing list of update records in the table, are you suggesting that we always show the entire table sorted by when the most recent rows were added? So if the user wanted to they could keep scrolling back to see older changes?

If we want to be able to show the 'ringing bell' when there are changes the user hasn't seen then we could store the latest changes table record id against each user instead of the last viewed timestamp, most of the work for that is already done.

So to summarize:

  • Index items in whatsnew.json by version number instead of date
  • Hook the plugin update logic to check for new whatsnew.json items by version and push to a new table 'whats_new'.
  • The What's New dialog should simply show a list of everything in the table (with pagination) sorted by most recent records
  • Store the whats_new table last record id against the user (instead of a timestamp) whenever they view the 'What's new' dialog and use that to determine whether to show a 'ringing bell' icon for new changes or just the non-ringing one if not.

Sounds good to me, are you happy with this spec?

@tsteur commented on November 5th 2021 Member

From a UI perspective we'd still have a growing list of update records in the table, are you suggesting that we always show the entire table sorted by when the most recent rows were added? So if the user wanted to they could keep scrolling back to see older changes?

We could show say the entries from last 3 months. If there are less than 10 entries, then we could show at least 10 entries.

If we want to be able to show the 'ringing bell' when there are changes the user hasn't seen then we could store the latest changes table record id against each user instead of the last viewed timestamp, most of the work for that is already done.

Sounds great 👍

Overall sounds also all good 👍

@bx80 commented on November 11th 2021 Contributor

This branch has now been updated and this issue is ready for review.

There is a new event which fires whenever a plugin is updated or installed. It is caught by CoreHome which then checks the affected plugin folder for a changes.json file. If this is found then any changes contained therein will be loaded into a new 'changes' database table. A unique index will prevent changes being loaded multiple times so version number checking can be avoided, this reduces complexity and potential processing problems with unknown plugin version formats.

The 'What's new' icon on the menu bar will be hidden entirely if there are no changes at all in the table. The popup will show changes in the order they were added to the table, latest first. Changes added more than 90 days ago will not be shown unless there would be less than 10 changes shown, in which case the most recent 10 changes are shown.

When a user views the 'What's new' popup then the maximum change table id shown to them is immediately stored against their user record. If the changes table contains records with higher sequence id then the 'What's new' icon will show as a ringing bell to indicate that there are unviewed changes, otherwise it will be a plain bell.

Although the version number and plugin name are not used for anything at the moment, it seems sensible to have them in the table in case there is a need to display this data or add additional filters later on.

To manually test plugin updates I did the following:
1) Added a changes.json file to a random plugin (RerUserDates)
2) Commented out the plugins/CorePluginsAdmin/PluginInstaller.php line 331 $this->removeFolderIfExists($pluginTargetPath); to prevent the plugin upgrade wiping the directory
3) Edited the plugin.json and changed the version number to something lower than the current marketplace version
4) Upgraded the plugin from the marketplace
This will cause the changes to be detected and loaded into the changes table.

This is a sample changes file:

[
  {"title":       "New feature x added",
   "description": "Now you can do a with b like this",
   "linkName":    "For more information go here",
   "link":        "https://www.matomo.org",
   "version":     "4.0.2"
  },
  {"title":       "New feature y added",
   "description": "Now you can do c with d like this",
   "linkName":    "For more information go here",
   "link":        "https://www.matomo.org",
   "version":     "4.0.1"
  },
  {"title":       "New feature z added",
   "description": "Now you can do e with f like this",
   "linkName":    "For more information go here",
   "link":        "https://www.matomo.org",
   "version":     "4.0.0"
  }
]

UI and integration tests have been updated.

@bx80 commented on November 11th 2021 Contributor

Just thinking about translations. Currently whatever text is in the changes.json will be shown in 'What's new' regardless of the user's chosen language. There should be a way for plugin authors to provide a list of changes in multiple languages.

Options could be:

  1. Don't do anything about this now.
  2. Look for changes.en.json, changes.de.json, etc and load them all into the table with a new column to denote language, then 'What's New' can filter the changes table by the current user's language setting. Easy to do but bypasses the current translations structure and if there are no translations for the chosen language then no changes would be shown.
  3. Still have one changes.json but all the text values are translation keys, store the translation keys in the changes table and then translate them in the template at run time. If there are no translations available then the user would be shown broken layout, maybe fall back to English?

Or maybe there is a better way? Thoughts welcome! :slightly_smiling_face:

@github-actions[bot] commented on November 26th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@justinvelluppillai commented on December 7th 2021 Contributor

Hey @bx80 if you've resolved the issues above can you please mark them resolved? Then maybe be good to request review directly on this one to get it moving along.

@bx80 commented on December 8th 2021 Contributor

All issues should now be resolved, except for whether to keep the user 'last changes viewed' value stored as a new column on the user table or store as a row in the options table. If a decision can be made on this then I can make the change if required.

@bx80 commented on December 9th 2021 Contributor

After discussion with @tsteur we won't implement translations for the 'What's new' feature at the moment.

This PR is now be ready for final review.

@bx80 commented on December 16th 2021 Contributor

Sorry about the accidental UI screenshots :facepalm:, I've done a commit to revert them.

I've added a sample changes.json file to the example plugin, one with a link and one without to show it's okay not to have a link. These changes are present in integration testing which is why the changes test was tweaked slightly.

@sgiehl commented on December 17th 2021 Member

@bx80 there are still some tests failing. One last feedback:

  • The links in the what's new popover don't have a special styling. Might be good to have it directly visible that it's a link.

Otherwise this would be good to merge from my point of view. But not sure if maybe @tsteur wants to have another quick look to check if it fulfills the requirements.

Btw. Are we planning to also post some changes in core with each version? If so we maybe should also have a separate changes file for core maybe, as not every feature can be mapped to a specific plugin though.

@bx80 commented on December 20th 2021 Contributor

@sgiehl The tests related to this change should all be passing now. There are three UI tests still failing but I don't think they are related.

Good point about link visibility. I've added some styling to match what we typically have elsewhere for 'more info' links.

For core/general changes I was thinking we could put them in the changes.json for the CoreHome plugin which would avoid the need to check for another file. I've tweaked the whatIsNew template to not prefix any CoreHome changes with the plugin name, so they will look like general Matomo changes.

@tsteur commented on December 20th 2021 Member

The only thing I would maybe change is a slightly different background color similar to what we have in the cloud.

It looks like we're actually using a Modal there and not a popover. And it has the background background-color: #fafafa;
image

it could still be a popover if we could ideally change the background color.

Also noticed in Mobile maybe the width is too wide? not sure if that's maybe a general Popover issue or if we specify the width somewhere.
image

The modal adjusts automatically by the looks
image

Maybe it's easier to use Modal? Sorry about bringing this up so late in the process.

Also some events would be good so that Cloud can filter out certain messages for specific plugins or so.

@tsteur commented on December 20th 2021 Member

BTW what I find helpful too would be already documenting it on https://github.com/matomo-org/developer-documentation/
Sometimes when documenting something then it helps finding things that are maybe complicated to explain and then I sometimes realise need to simplify something. Not saying that's the case here or that it will be. Sometimes that's not the case. I'm just meaning in past it helped me find issues if there are any as then often the docs are hard to explain.

I wonder if we should maybe rename this page https://developer.matomo.org/guides/extending-database to something more generic "Providing Updates" related and mention it there? We could then later also document there how to perform config migrations etc.

@bx80 commented on December 22nd 2021 Contributor

@tsteur I couldn't find a straightforward way to switch to a modal instead of a popover, we don't seem to have a modal based equivalent of Piwik_Popover.createPopupAndLoadUrl() and it seemed an overkill to create a vue component just for this. Instead I added some styling to the popover to set the background colour and adjust to the width dynamically based on screen size.

I've added a new Changes.filterChanges event which provides an array of changes that are about to be shown, so Cloud can catch this event and filter out any changes that should be hidden. To allow this filtering to work consistently I modified the doChangesExist() code to call getChangesItems() rather than using a separate count query, otherwise changes could be found by the count query causing the notification icon to be shown, but then when showing the popup those changes could have been removed by the filter event.

PR 596 has been submitted for review to the developer docs repository with an explanation of how the changes.json works. I didn't rename the Extending the database section as it seems to be very much about using the database in code rather than updates. Maybe a separate Providing updates section for change notifications and migrations could work?

@tsteur commented on December 22nd 2021 Member

👍 sounds good, we can have a separate Providing updates section

All good re popover vs modal

This Pull Request was closed on December 22nd 2021
Powered by GitHub Issue Mirror