@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.

@alikon Why do you remove the privacyconsent plugin with your latest commit? As far as I can see it should not be removed completely because your PR here only replaces a part of its functionality. With your other PR for the logrotation it is different: There you can remove the old plugin because your new PR replaces that completely. But here that's not the case, or am I missing something?

this replaces all the functionalities of privacyconsent

this replaces all the functionalities of privacyconsent

no it doesn't, please have a look at the plugin you deleted and all it events it has.

you both are right sorry

@alikon Does it need to remove some obsolete configuration parameters from the "plugins/system/privacyconsent/privacyconsent.xml" file?

yes done

@alikon We have 2 fields with name="consentexpirationdays" and label="PLG_TASK_PRIVACYCONSENT_CONSENTEXPIRATIONDAYS_LABEL" and description="PLG_TASK_PRIVACYCONSENT_CONSENTEXPIRATIONDAYS_DESC", one in file "plugins/task/privacyconsent/forms/invalidateForm.xml" and the other one in file "plugins/task/privacyconsent/forms/remindForm.xml". Is that right?

@alikon What's also not clear to me is what replaces the old "cachetimeout" parameter.

@alikon What's also not clear to me is what replaces the old "cachetimeout" parameter.

It has been replaced by Execution Rule / interval /cron expression

@alikon We have 2 fields with name="consentexpirationdays" and label="PLG_TASK_PRIVACYCONSENT_CONSENTEXPIRATIONDAYS_LABEL" and description="PLG_TASK_PRIVACYCONSENT_CONSENTEXPIRATIONDAYS_DESC", one in file "plugins/task/privacyconsent/forms/invalidateForm.xml" and the other one in file "plugins/task/privacyconsent/forms/remindForm.xml". Is that right?

i think so, cause 1 is for delete expired consents and the other 1 is for remind about near to expire consent

@alikon We have 2 fields with name="consentexpirationdays" and label="PLG_TASK_PRIVACYCONSENT_CONSENTEXPIRATIONDAYS_LABEL" and description="PLG_TASK_PRIVACYCONSENT_CONSENTEXPIRATIONDAYS_DESC", one in file "plugins/task/privacyconsent/forms/invalidateForm.xml" and the other one in file "plugins/task/privacyconsent/forms/remindForm.xml". Is that right?

i think so, cause 1 is for delete expired consents and the other 1 is for remind about near to expire consent

@alikon How can these 2 parameters with the same name then be distinguished? Have you tried to save the configuration of your new plugin with 2 different values for these parameters and then checked the value of the params column of the record of your new plugin in the #__extensions table in database? If so, and everything works, could we provide the initial value for that column in the base.sql for new installations as well as in the update SQL when we add the new plugin on update?

How can these 2 parameters with the same name then be distinguished?

@richard67 consider that these are 2 separate task event let me share a screen:

for expiration

and for remind

@alikon I see. You have different tasks. But it seems you are not adding these tasks when making a new installation or an update, i.e. no insert into the #__scheduler_tasks table. So how does it work? Do these tasks have to be created manually?

this must be discussed imho, so i've left out for the moment

this must be discussed imho, so i've left out for the moment

@alikon What has to be discussed is the migration of existing settings from the old plugin to the new plugin, which might include updating the previously created new tasks. But the tasks should be created already in the extensions.sql for new installations when the corresponding functionality before this PR here was enabled for new installations, and it should also be done in the update SQL when privacy consents were enabled on update when we had introduced them once in past.

@alikon And still I don't understand why we have to set the "Expiration" parameter in both tasks. What happens if I set them differently? I.e. the expiration will happen after 180 days and the reminder will be based on an expiration after 360 days? That would not really make sense, would it?

@richard67

But the tasks should be created already in the extensions.sql for new installations

i'm would like to not set anything for plugins (scheduled task) on new installations, but that's my personal opinion, that should be an informed action by the admin

why we have to set the "Expiration" parameter in both tasks

good valid point

i'll convert to draft for now ... it needs more care for sure

ready for review

ready for review

@alikon I think it needs a migration of parameters on update when people have the old privacy consent system plugin enabled.

In opposite to other PRs for migrating stuff to tasks we do not have to delete the old plugin here, so the migration of parameters cannot be done with the "uninstallExtensions" method, it has to be called in the "postflight" method like we already do it for the migration of the TinyMCE editor plugin's options.

I.e. create a new function, e.g. "migratePrivacyconsentConfiguration", and call that in "postflight" in the same way as we call the "migrateTinymceConfiguration" function, i.e. add following code after here https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L948 :

        if (!$this->migratePrivacyconsentConfiguration()) {
            return false;
        }

The migration should check the "enabled" status of the old plugin and only migrate something if that status is 1 (enabled).

missed that one https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L944

?

missed that one https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L944

@alikon It will also need to change the version number here https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L940 to 5.0.1, similar as we did here https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L216-L219 , and adapt the comment to that here https://github.com/joomla/joomla-cms/blob/5.0-dev/administrator/components/com_admin/script.php#L944 . Otherwise the migration would not run when updating from a 5.0.0-alpha1 or alpha2.

Not sure if we should do it with this PR here or if I should do it with a separate PR.

...Otherwise the migration would not run when updating from a 5.0.0-alpha1 or alpha2.
Not sure if we should do it with this PR here or if I should do it with a separate PR.

as long as we don't have certainty when and if it will be merged....a later pr maybe

@alikon I've allowed myself to update your branch to latest 5.0-dev and fix the conflict. Please pull the changes before you continue to work on your local clone.

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

@HLeithner As far as I can see, this has been implemented now. Could you approve the change or dismiss your review so that GitHub doesn't show it anymore as unresolved conversation? Thanks in advance.

@alikon Could you rename the update SQL scripts to something newer than "5.0.0-2023-07-12.sql" (= newest update SQL right now in the 5.0-dev branch), e.g. to "5.0.0-2023-07-25.sql"? Thanks in advance.

@alikon It needs again to rename the update SQL scripts, this time to something newer than "5.0.0-2023-08-21.sql". I suggest to use "5.0.0-2023-08-23.sql" for this PR here.

done

Task for new installations is missing

Task for new installations is missing

@heelc29 I think that's not needed because on a new installation of J5 without this PR, the privacy consent is disabled as far as I know, and we should keep the functionality and not create a task on a new install. When updating it depends on the previous enabled status of the privacyconsent plugin if we create a new task or not. Other PRs where something is replaced which would be enabled on a new install will create also a task for new installations. But that's not the case here.

@alikon I've solved the merge conflict caused by the recently merged globalcheckin task plugin, so before you continue to work locally on your branch you have to pull the changes from remote.

@alikon I've allowed myself to update the branch and apply some review changes. Now I think it needs better testing instructions. I suggest something as follows for this PR here, and will later make suggestions also for the other 4 PRs. Feel free to just copy it and paste into the descriptions of the PRs, or modify them as you want.

Testing Instructions

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.

Remove backslash:

} catch (Exception $e) {

\Exception

@sandewt That should be fixed with a separate PR because this code was not added by the PR here. It was my mistake btw. because it was me who once had added that function. Do you want to make a PR for a fix? Otherwise I will do later. But it has a low priority for me as it's just a code style thing more or less. Both works.

@sandewt That should be fixed with a separate PR because this code was not added by the PR here.

Done, see pr #41547

@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 this one, too? Testing instructions can be found in the description of this PR. Thanks in advance.

thank you all for your help

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

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

joomla-cms/plugins/task/privacyconsent/src/Extension/PrivacyConsent.php

Lines 127 to 132 in 087821f

	$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)?

I've asked the other maintainers to check that.

@alikon Could you check the above comments and advise?

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

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?

yes i'll do

@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.

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

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.

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

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

@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?

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

? good team working