-
Notifications
You must be signed in to change notification settings - Fork 72
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
Remove the deprecated method Tools::jsonDecode
, Fix Reset & Bump to 1.4.1
#165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Progi1984 ,
Thanks for the PR !
Tested upgrade to 1.4.1, install, configuration, reset, disable/enable, uninstall.
On PS develop branch (on branch 1.7.8.x it is OK), I get an error when I try to Get my data to PDF or CSV from my customer account 🚫 :
Screen.Recording.2022-03-29.at.11.05.35.mov
Could you check ?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced by variables replacement. Looks harder to debug. (using variables invite to separate in smaller functions)
Not blocking for me :)
Good to remove Tools::jsonDecode
0aa9194
to
ce465df
Compare
ce465df
to
8813cc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Progi1984 ,
Tested on 1.7.8.x and develop.
In BO, tested upgrade, reset, disable/enable on desktop and mobile, configuration of module.
Checked in FO in customer account "get your data" buttons and consent checkboxes.
LGTM, it is QA ✅
Thank you @Progi1984 @florine2623 |
Tools::jsonDecode
* Reset the module (no problem anymore)
* Go to configure page (no problem anymore)
* Release Test