Skip to content

Allow guests to re-subscribe after unsubscribing them from the module configuration#95

Open
Orakell wants to merge 1 commit into
PrestaShop:devfrom
wepika:delete-guest-subscription
Open

Allow guests to re-subscribe after unsubscribing them from the module configuration#95
Orakell wants to merge 1 commit into
PrestaShop:devfrom
wepika:delete-guest-subscription

Conversation

@Orakell

@Orakell Orakell commented Mar 3, 2023

Copy link
Copy Markdown
Questions Answers
Description? When you unsubscribe a visitor subscribed to the newsletter via the "subscriber" checkbox in the module configuration listing (see screenshot), the "active" field changes to false in the DB. This prevents the visitor from re-subscribing to the newsletter. If the visitor unsubscribes himself, the line is deleted in the DB and he can re-subscribe.
Type? bug fix / improvement
BC breaks? no
Deprecations? no
Fixed ticket? none
How to test?
  • Subscribe to the newsletter as a guest
  • Un-subscribe yourself from the newsletter in the module configuration
  • Re-subscribe to the newsletter as a guest with the same email

image

@matthieu-rolland

Copy link
Copy Markdown
Contributor

thank you for your contribution @Orakell 👍

before validating this kind of PR we need you to create an issue, with a step by step way of reproducing the bug

then the QA team will reproduce the bug and we can validate your work 👍

@Lionel-dev

Copy link
Copy Markdown

I just reproduced the bug on the version of PrestaShop 8.0.1 and with the module in 2.7.1

I also took advantage of the patch and according to me everything is ok. Nice patch @Orakell !

@Orakell

Orakell commented Mar 3, 2023

Copy link
Copy Markdown
Author

Thanks :)

And here is the issue : PrestaShop/PrestaShop#31639

@Hlavtox

Hlavtox commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

Change is nice, but we should check the behavior, just so we don't introduce some nasty behavior change. The subscriber was in the DB with active = 0 for a reason (probably :D).

@PrestaEdit

Copy link
Copy Markdown
Contributor

I will say that the must will be to :

  • Kept this behavior, it's surely there for something and a merchant could want to disallow the subscription (like a RGPD ask) and if you delete it, you will allow someone to register your email even if not wanted.
  • Add a RowAction to allow a real delete.

@kpodemski kpodemski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @Orakell

I agree with @PrestaEdit.

If he wants to subscribe again, we should see if there's a customer email with active = 1. If not, we can subscribe to him by adding a new email or changing active from 0 to 1 for his record in the database.

Having a chance to really delete the records would be a "nice-to-have".

What do you think?

@Orakell

Orakell commented Mar 29, 2023

Copy link
Copy Markdown
Author

Hello :)

It was odd to me that it does not delete the row when the admin unsubscribe the guest manually.

Because when the unregister function is called, the row is deleted :

protected function unregister($email, $register_status)
{
    if ($register_status == self::GUEST_REGISTERED) {
        $sql = 'DELETE FROM ' . _DB_PREFIX_ . 'emailsubscription WHERE `email` = \'' . pSQL($_POST['email']) . '\' AND id_shop = ' . $this->context->shop->id;
    } elseif ($register_status == self::CUSTOMER_REGISTERED) {
        $sql = 'UPDATE ' . _DB_PREFIX_ . 'customer SET `newsletter` = 0 WHERE `email` = \'' . pSQL($_POST['email']) . '\' AND id_shop = ' . $this->context->shop->id;
    }

    if (!isset($sql) || !Db::getInstance()->execute($sql)) {
        return false;
    }

    return true;
}

@kpodemski

Copy link
Copy Markdown
Contributor

Hi @Orakell

True :( There's a mix of everything there :(

@HartLarsson

Copy link
Copy Markdown

that is the right fix! thanks!

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.

7 participants