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

GDPR Consent log #188

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

NicolasCador
Copy link

@NicolasCador NicolasCador commented Feb 11, 2023

Questions Answers
Description? Make Log working for any module.
...
Type? bug fix / improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#31317
How to test? Go to BO > Modules > GDPR and click on configure

Click on tab "Data Consent"

Enable GDPR on any module and Save

In FO, Check GDPR box and click on Submit

Go back to BO > Modules > GDPR > Data Consent

There is a log or registration or proof of your GDPR acceptance.

@fox-john
Copy link
Contributor

fox-john commented Apr 5, 2023

Hi @NicolasCador,

The module has has been completely reworked on the PHP code side. You will probably need to rebase your work and check the compatibility of your code with the current code.

@AlmostDigital
Copy link

Hello everyone, regarding the latest update I'm experiencing a db query error, the table prefix is written in text within the findAll() method and this generates an exception for table not found.

Screenshot 2023-04-07 alle 13 30 38

I think it is necessary to include the constant DB_PREFIX,
best regards

@NicolasCador
Copy link
Author

Hi @fox-john,

yes, you're right, huge job done !

I can re-propose my bug fix.

@NicolasCador NicolasCador reopened this Apr 19, 2023
@NicolasCador
Copy link
Author

Hi @hibatallahAouadni,

I updated my proposition following 2.0.1 release.

@hibatallahAouadni hibatallahAouadni requested a review from a team April 25, 2023 00:58
@NicolasCador
Copy link
Author

Hi all,

I do not understand why it found an error on line 138 of ExportService.php.
It works on my site.

Or do I not see?

Could you please help me?

@fox-john
Copy link
Contributor

fox-john commented May 10, 2023

Hi all,

I do not understand why it found an error on line 138 of ExportService.php. It works on my site.

Or do I not see?

Could you please help me?

Hello @NicolasCador ! You need to add extra space beetween your point separator, after _PS_CORE_DIR_ and before $moduleData->template;

And for PHPStan, you need to cast your variable, line 138 (actually is array|string).

Line   src/Service/ExportService.php                      
------ --------------------------------------------------- 
138    Cannot access property $template on array|string.
------ --------------------------------------------------- 

@NicolasCador
Copy link
Author

Hi @fox-john,

ok, thank you very much, I updated.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Hello @NicolasCador I try to review your Pull Request but it's very hard for me to understand the relationship between your code changes and the goal. Can you please guide me?

@NicolasCador
Copy link
Author

NicolasCador commented May 12, 2023

Hi @matks,
yes of course.

  • I updated controllers/front/FrontAjaxGdpr.php to be in compliance with rules:
    • ModuleFrontController instead of FrontController
    • displayAjaxAddlog instead of if (Tools::getValue('action') !== 'AddLog') {...
  • I updated controllers/front/gdpr.php and suppressed views/js/front.js because front.js is useless with just an ajax call with action=test never used anywhere and depending on a click on something with id=exportPersonalData, implemented nowhere.
  • I updated psgdpr.php and views/templates/hook/display_rgpd_consent.tpl to :
    • make easier the understanding of url
    • ensure the ajax call will be done only on the right click with "parentForm.on('submit', function(event) {" instead of "$(document).on('submit', parentForm, function(event) {", because currently, if you click on "send" in contact form, it is also sending a second call for "newsletter inscription"
    • make sure the ajax call will be done with success due to enough time: I added "async: false,". If not, the call is randomly OK, depending on the time spent of the call.
  • Then I updated src/Service/ExportService.php and views/templates/front/pdf/sections/modules.tpl to allow developers to use their own Template for the pdf export, because it could be fun to be able to put data under a customized format especially when there are a lot of data to be exported. It's where the test fails... As we collect data through a json flow, to be retrocompatible I test if we have an array, it's as today without a template, if we have an object, I manage the template name. As the PHPStan test doesn't understand this subtlety, I think I will force the json collect as array, and then check if there is an index ['template'].

@NicolasCador
Copy link
Author

Hi @kpodemski ,

ok, I added the 2 spaces required by the failed test.

Could you please relaunch a test?

Thanks

@kpodemski
Copy link
Contributor

@NicolasCador I just did, thanks Nicolas :)

@NicolasCador
Copy link
Author

Hi @kpodemski ,

ok, I suppressed the line and the useless spaces required by the failed test.

Wow, it's so precise! Sorry

Could you please relaunch a test?

Thanks

@NicolasCador NicolasCador requested a review from matks May 26, 2023 12:16
@kpodemski
Copy link
Contributor

@fox-john could you take a look and review the PR?

@NicolasCador
Copy link
Author

Hi @fox-john , @matks

could you take a look and review the PR?

@matks
Copy link
Contributor

matks commented Aug 16, 2023

I will try to do it today or tomorrow ;)

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

@NicolasCador OK I understood why I was confused ^^

I did not understand how the code changes you suggest would fix issue PrestaShop/PrestaShop#31317 . But the issue PrestaShop/PrestaShop#31317 is already solved: I just verified. I followed the issue steps and I am able to see the consent being registered.

So @NicolasCador your PR does not fix the problem (it is already fixed), it is now a refactoring. Is that right? You improve how the code is written but you do not modify the behavior.

@NicolasCador
Copy link
Author

Hi @matks,

there hasn't any change since my PR in the module code, so no, the PrestaShop/PrestaShop#31317 is not already fixed:
While "$(document).on('submit', parentForm, function(event) {" is still for ajax call in views/templates/hook/display_rgpd_consent.tpl, if you have 2 modules on the same page (like ps_emailsubscription and productcomments) using psgdpr, the log will log 2 actions, while only one has been sent.
And while you don't have "async: false," in the ajax call, you are not sure the action will be logged (depending on the server speed).

As I explained in #188 (comment), the other codes are for cleaning, or make compliant with rules, and to allow developers to use their own Template for the pdf export, because it could be fun to be able to put data under a customized format especially when there are a lot of data to be exported.

We can have a small videoconference if necessary to discuss.

@tashkasnamai
Copy link

gdpr
Can somebody fix this one, translations not working.

@randshell
Copy link

Are there any updates on the progress of this issue?

@tashkasnamai
Copy link

Are there any updates on the progress of this issue?

yeah download from this page its fixed here

@NicolasCador NicolasCador requested a review from matks November 25, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No GDPR Consent log for any modules
7 participants