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

[5] add2scheduler-updatenotification #40788

Merged
merged 89 commits into from
Sep 3, 2023

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Jun 18, 2023

Summary of Changes

move update notification from system plguin to scheduler/task plugin

Testing Instructions

Hint: The execution times and hours shown for tasks in the administrator are in the UTC timezone.

New installation

Make a new installation with this PR applied.

Check if there is an enabled task scheduler plugin "Task - Joomla! Update Notification".

Check if there is an enabled scheduled task "UpdateNotification" using that plugin.

Check that the new task shall run every 24 hours at the hour when the Joomla installation was made.

Update

On a Joomla 4.4-dev version or 4.4.0 alpha 4, note the endabled status and the configuration parameters of the "System - Joomla! Update Notification" plugin.

Now either disable that plugin or enable it and optionally change some of the configuration parameters to a value different to the default.

Update to the patched package or custom update URL created by Drone for this PR.

Check if the "System - Joomla! Update Notification" plugin has been uninstalled.

Check if there is a task scheduler plugin "Task - Joomla! Update Notification".

Check enabled status and configuration parameters of that plugin.

Check if there is a scheduled task "UpdateNotification" using that plugin. If so, check the configuration parameters, too.

Repeat the previous steps with different endabled status and configuration parameters of the "System - Joomla! Update Notification" plugin.

Actual result BEFORE applying this Pull Request

Notification on available Joomla updates is done with the "System - Joomla! Update Notification" plugin.

Expected result AFTER applying this Pull Request

New installation

The "Task - Joomla! Update Notification" plugin is enabled, and a scheduled task "UpdateNotification" is enabled to run every 24 hours at the hour of the Joomla installation.

Update

The "System - Joomla! Update Notification" plugin has been uninstalled by the update.

A new task scheduler plugin "Task - Joomla! Update Notification" has been created and is enabled.

If the old "System - Joomla! Update Notification" plugin was enabled before the update, a new scheduled task "UpdateNotification" has been created and is enabled.

The configuration parameters "Super User Emails" and "Email Language" of that task are set according to the old system plugin's parameters.

The interval hours is equal to the cache timeout parameter in the configuration option of the "Installer" component (com_installer).

If the old "System - Joomla! Update Notification" plugin was disabled before the update, there is no task for that plugin.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org: link will be added later

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@alikon alikon changed the base branch from 4.3-dev to 5.0-dev June 18, 2023 10:19
@alikon alikon mentioned this pull request Jun 18, 2023
4 tasks
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators and removed Unit/System Tests labels Jun 18, 2023
alikon and others added 2 commits June 18, 2023 13:01
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@alikon alikon marked this pull request as ready for review June 25, 2023 06:57
@richard67
Copy link
Member

@alikon Besides my review suggestions following is missing:

  1. The files of the old "plg_system_updatenotification" plugin have to be deleted in the source code, i.e. in Git/GitHub. This should be done with this PR here. For the lists of deleted files and folders in script.php I will care afterwards with a separate PR.
  2. Currently, i.e. without this PR, when you make a new installation, the old "plg_system_updatenotification" plugin is enabled by default. Now with your PR there will not be a task after a new installation. That's a loss of functionality.
    To fix this, it needs to add an insert statement for the task in file "extensions.sql" after the create table #__scheduler_tasks.
    In order not to have all new installations in the world checking for updates at the same time of the day, we could use NOW() and datetime formatting to get the execution hour. I have already ideas and can prepare something in a PR for you.
    The question is if we shall insert the new task with asset_id zero, or if we shall also create an asset for it in base.sql. I think asset_id zero should work so an asset would be created when editing and saving the task parameters in backend.

Point 2 is only needed when the functionality is enabled on a new installation by default, like it is here and for the logrotation. For things which are not enabled by default on a new installation, like e.g. the privacy consents, we would not need that.

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67
Copy link
Member

@heelc29 The PR is ready for testing. Code style error is not related to the PR but to the current 5.0-dev branch. Could you test again? Testing instructions can be found in the description of this PR. Thanks in advance.

@richard67
Copy link
Member

I have tested this item ✅ successfully on c3ff33a

Done all the tests - new install and update - on MySQL 8 and PostgreSQL 14.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40788.

@alikon
Copy link
Contributor Author

alikon commented Sep 4, 2023

thank you all for your help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants