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-privacyconsents #40553

Merged
merged 88 commits into from
Sep 3, 2023

Conversation

alikon
Copy link
Contributor

@alikon alikon commented May 7, 2023

Summary of Changes

  • add the plg_system_privacyconsents feature "Delete the expired privacy consents" to a proper scheduled task plugin
  • add the plg_system_privacyconsents feature "Remind for privacy consents near to expire" to a proper scheduled 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 - Privacy Consents".

Check if there is any scheduled task using that plugin.

Update

On a Joomla 4.4-dev version or 4.4.0 alpha 4, note the endabled status and the configuration parameters in the "Expiration" fieldset of the of the "System - Privacy Consent" plugin.

Now either disable that plugin or enable it and optionally change some of the configuration parameters in the "Expiration" fieldset 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 "Expiration" fieldset of the "System - Privacy Consent" plugin has been removed.

Check if there is a task scheduler plugin "Task - Privacy Consents".

Check enabled status and configuration parameters of that plugin.

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

Repeat the previous steps with different endabled status of the "System - Privacy Consent" plugin and - if enabled - different configuration parameters in the "Expiration" fieldset of that plugin.

Actual result BEFORE applying this Pull Request

Reminders on and deletion of expired consents are done with the "System - Privacy Consent" plugin.

Expected result AFTER applying this Pull Request

New installation

The "Task - Privacy Consents" plugin is enabled.

There is no scheduled task using that plugin because on a new installation the "System - Privacy Consent" is disabled.

Update

The "Expiration" fieldset has been removed from the configuration parameters of the "System - Privacy Consent" plugin by the update.

A new task scheduler plugin "Task - Privacy Consents" has been created and is enabled.

If the "System - Privacy Consent" plugin was enabled and field "Enabled" was set to "Yes" in the "Expiration" fieldset of that plugin before the update, a new scheduled task "PrivacyConsent" has been created and is enabled.

The task executes every n days, with n being the "Periodic check (days)" value which was set in the "Expiration" fieldset of the system plugin before the update, default 30.

The configuration parameters "Expiration" and "Remind" of that task are set to the same values as the same parameters in the in the "Expiration" fieldset of the old system plugin.

If the old "System - Privacy Consent" plugin was disabled or field "Enabled" was set to "No" in the "Expiration" fieldset of that plugin 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

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-5.0-dev labels May 7, 2023
alikon and others added 3 commits May 7, 2023 13:03
Co-authored-by: jsanders <j53.sanders@gmail.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
alikon and others added 2 commits May 7, 2023 13:14
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
alikon and others added 2 commits May 7, 2023 13:26
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, can you please remove the functionallity from the plg_system_privacyconsents plugin please.

plugins/task/privacyconsent/forms/invalidateForm.xml Outdated Show resolved Hide resolved
plugins/task/privacyconsent/forms/remindForm.xml Outdated Show resolved Hide resolved
@alikon alikon marked this pull request as draft June 10, 2023 08:19
@richard67
Copy link
Member

richard67 commented Jun 11, 2023

@alikon What is missing is the SQL changes in base.sql to add the new task plugin to the extensions table (if possible with reasonable default params) and an update SQL script for doing the same. Use the newest present update SQL which does an insert into the extensions table as example, and give the files the right name, e.g. "5.0.0-2023-06-12.sql". It could also make sense to use one common update SQL for this PR and the other one #40519 . The new plugin needs also to be added to the extensions helper.

@richard67 richard67 mentioned this pull request Jun 11, 2023
4 tasks
@HLeithner HLeithner merged commit 82b790f into joomla:5.0-dev Sep 3, 2023
HLeithner pushed a commit that referenced this pull request Sep 3, 2023
* add2schedulerdeleteactionlogs

* cs

* Rename 5.0.0-2023-06-27.sql to 5.0.0-2023-08-05.sql

shift

* Rename 5.0.0-2023-06-27.sql to 5.0.0-2023-08-05.sql

shift

* Rename 5.0.0-2023-08-05.sql to 5.0.0-2023-08-25.sql

* Rename 5.0.0-2023-08-05.sql to 5.0.0-2023-08-25.sql

* Update plugins/task/deleteactionlogs/src/Extension/DeleteActionLogs.php

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* blank-line

* blank-line

* blank-line

* remove config node

* Rename 5.0.0-2023-08-25.sql to 5.0.0-2023-08-30.sql

* Rename 5.0.0-2023-08-25.sql to 5.0.0-2023-08-30.sql

* Deprecate language strings

* Single quotes

* Remove extra reference assignment for dispatcher argument

* Do it in the same way as in PR #40553

* Rename 5.0.0-2023-08-30.sql to 5.0.0-2023-09-02.sql

* Rename 5.0.0-2023-08-30.sql to 5.0.0-2023-09-02.sql

* Fix lastrun default on update

* CS

* deploy version

* Do nothing if params is an empty JSON

* Fix migration methods and comment typos

* Use the right task type

* Fix undefined array element "exec-day"

---------

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>
Co-authored-by: Richard Fath <richard.fath@t-online.de>
@alikon alikon deleted the add2scheduler-privacy branch September 4, 2023 17:11
@alikon
Copy link
Contributor Author

alikon commented Sep 4, 2023

thank you all for your help

@heelc29
Copy link
Contributor

heelc29 commented Sep 4, 2023

@richard67 Should some content of '#__privacy_consents' updated (content of subject) during upgrade to J5?

$query->select($db->quoteName(['r.id', 'r.user_id', 'u.email']))
->from($db->quoteName('#__privacy_consents', 'r'))
->join('LEFT', $db->quoteName('#__users', 'u'), $db->quoteName('u.id') . ' = ' . $db->quoteName('r.user_id'))
->where($db->quoteName('subject') . ' = ' . $db->quote('PLG_TASK_PRIVACYCONSENT_SUBJECT'))
->where($db->quoteName('remind') . ' = 0')
->where($query->dateAdd($db->quote($now), $period, 'DAY') . ' > ' . $db->quoteName('created'));

@richard67
Copy link
Member

@richard67 Should some content of '#__privacy_consents' updated (content of subject) during upgrade to J5?

$query->select($db->quoteName(['r.id', 'r.user_id', 'u.email']))
->from($db->quoteName('#__privacy_consents', 'r'))
->join('LEFT', $db->quoteName('#__users', 'u'), $db->quoteName('u.id') . ' = ' . $db->quoteName('r.user_id'))
->where($db->quoteName('subject') . ' = ' . $db->quote('PLG_TASK_PRIVACYCONSENT_SUBJECT'))
->where($db->quoteName('remind') . ' = 0')
->where($query->dateAdd($db->quote($now), $period, 'DAY') . ' > ' . $db->quoteName('created'));

Possibly. Can't check now. But the query looks as if it would fail for the old consents created with the old subject. Maybe it would be easier to extend this query so it checks for both subjects (language strings)?

@richard67
Copy link
Member

I've asked the other maintainers to check that.

@richard67
Copy link
Member

@alikon Could you check the above comments and advise?

@alikon
Copy link
Contributor Author

alikon commented Sep 4, 2023

could be a solution to add an update to the '#__privacy_consents' table on administrator/components/com_admin/sql/updates/mysql/5.0.0-2023-09-02.sql

@richard67
Copy link
Member

could be a solution to add an update to the '#__privacy_consents' table on administrator/components/com_admin/sql/updates/mysql/5.0.0-2023-09-02.sql

I think yes. Can you do that?

@alikon
Copy link
Contributor Author

alikon commented Sep 4, 2023

yes i'll do

@richard67
Copy link
Member

richard67 commented Sep 4, 2023

@alikon But I am not really sure. There is still the system plugin using a similar string for email subjects. I don’t know which of these or if both are relevant for the functionality of your task.

@alikon
Copy link
Contributor Author

alikon commented Sep 4, 2023

i'll check it better but no more PLG_SYSTEM_PRIVACYCONSENT_SUBJECT so far
so an update in the update script should do the work

@richard67
Copy link
Member

i'll check it better but no more PLG_SYSTEM_PRIVACYCONSENT_SUBJECT so far
so an update in the update script should do the work

@alikon I saw some code of the system plugin still using that string. So please double check. And if you make a PR please provide some testing instructions. I am too tired now already.

@alikon
Copy link
Contributor Author

alikon commented Sep 4, 2023

wrong grep or on the wrong barnch 😇
i'll check it better

@richard67
Copy link
Member

@alikon If the fix will not make it into beta 1 tomorrow, it should be in a new update SQL script.

@richard67
Copy link
Member

@alikon I could find the string PLG_SYSTEM_PRIVACYCONSENT_SUBJECT being used in the onUserAfterSave method of the system plugin here:

https://github.com/joomla/joomla-cms/blob/5.0-dev/plugins/system/privacyconsent/src/Extension/PrivacyConsent.php#L176-L188

It inserts a record with subject PLG_SYSTEM_PRIVACYCONSENT_SUBJECT into the #__privacy_consents table when the user's privacy consent confirmation is saved.

And here in the isUserConsented method it selects the count of records for a user with the same subject and state 1 to check if a user has consented:

https://github.com/joomla/joomla-cms/blob/5.0-dev/plugins/system/privacyconsent/src/Extension/PrivacyConsent.php#L384-L392

The new task scheduler plugin sends the remainder mails with the mail template. This uses PLG_TASK_PRIVACYCONSENT_EMAIL_REMIND_SUBJECT as subject for the emails. That subject is not related to the subject in the #__privacy_consents table.

The PLG_TASK_PRIVACYCONSENT_SUBJECT is only used by the new task scheduler plugin as where condition when querying the database for expired consent or for consent which will expire so a reminder mail has to be sent.

So I think the right fix will be to change the query in the task scheduler plugin to use PLG_SYSTEM_PRIVACYCONSENT_SUBJECT, and the language string PLG_TASK_PRIVACYCONSENT_SUBJECT can be removed if it is not used anymore after that change.

Do you agree?

@heelc29 Thanks for checking. What do you think? Am I missing something?

@richard67
Copy link
Member

@alikon @heelc29 Please check #41597 . Thanks in advance.

@alikon
Copy link
Contributor Author

alikon commented Sep 5, 2023

👍 good team working

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.

6 participants