Refactor hookActionCustomerAccountAdd in ps_emailsubscription - #121
Conversation
| $this->sendVoucher($email, $code); | ||
| } | ||
|
|
||
| return (bool) Db::getInstance()->execute('DELETE FROM ' . _DB_PREFIX_ . 'emailsubscription WHERE id_shop=' . (int) $id_shop . ' AND email=\'' . pSQL($email) . "'"); |
There was a problem hiding this comment.
| return (bool) Db::getInstance()->execute('DELETE FROM ' . _DB_PREFIX_ . 'emailsubscription WHERE id_shop=' . (int) $id_shop . ' AND email=\'' . pSQL($email) . "'"); | |
| return Db::getInstance()->execute('DELETE FROM `' . _DB_PREFIX_ . 'emailsubscription` WHERE id_shop = ' . (int) $id_shop . 'AND email = "' . pSQL($email) . '"'); |
There was a problem hiding this comment.
Or only ' with slash ' ?
There was a problem hiding this comment.
@Touxten, I based it on the previous code, but if you think it's better this way, I'll make the change.
There was a problem hiding this comment.
Yes, I saw that. What's causing me trouble is the mix of ' / ” at the end of the query.
I've validated the PR. I'll leave it up to you to choose :D Nice job !
There was a problem hiding this comment.
@Touxten I made the change you suggested, it does look better stylistically :-)
Thanks!
|
I'm going to do a PR to correct the tests. |
3756e51 to
b4ba7f6
Compare
|
@Touxten your PR worked perfectly! Thanks! |
|
ping @kpodemski |
paulnoelcholot
left a comment
There was a problem hiding this comment.
Hello @Codencode,
I tested your PR and I found some bug :
PR-2026-03-26_10.57.47.mp4
I have the problem when Double opt-in are activated or desactivated. And only for an account creation. I don't have this problem when I want to subscribe to the newsletter without creating an account.
Can you check that please?
Thanks!
|
Ping @Touxten @kpodemski |
|
@Touxten @kpodemski could you re-approve the PR with this fix in place? |
Tests performedScenario 1 — Account created without newsletter subscription
Result: ✅ The customer remains subscribed, and the email address is still present in the test-pr-email.mp4Scenario 2 — Account created with newsletter subscription
Result: ✅ The customer is subscribed through their customer account, and the email address is correctly removed from the test-pr-email-2.mp4@kpodemski and @Codencode Is this really what I was supposed to test? |
|
I wait before merge |
|
@Touxten While preparing the How to test, I noticed that when a customer creates an account and subscribes to the newsletter, the method returns before reaching This behavior already exists today, so the PR does not introduce it, but I’d like to confirm whether it is actually expected. If the email should be sent, I could update this PR to handle it, or would it be better to create a separate PR for that change? @kpodemski, what do you think? |
|
If the confirmation is required, then the email should be sent. My understanding is that this person shouldn't be considered as an active subscriber for as long as they haven't confirmed their subscription, correct? |
|
@kpodemski I was initially referring specifically to However, looking deeper into this, I found So I think there are actually two related cases here:
In my opinion, it would be better to merge this PR as it is and then open a separate PR to address both issues properly: sending the confirmation email and handling the verification email flow during account creation. What do you think? |
|
I agree—in any case, it has nothing to do with this PR; we'd have to create another one. |
|
Yes, I think so too. Let's wait for confirmation from @kpodemski, after which we can merge it and I'll create a separate PR. Actually, it's almost ready, so as soon as we merge this one I'll publish it so we can verify and merge it in a way that the new module release will also include the new fixes. |
|
This one is not related to my comment, merged 👍🏻 thanks guys |
|
New PR: #139 |
hookActionCustomerAccountAddlogic by acting only on valid emails and newsletter-subscribed customers, removing unnecessary and unreachable code.Related: #114 (comment)