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

Patch 1 #34

Merged
merged 11 commits into from Jul 8, 2019

Conversation

@Codencode
Copy link
Contributor

commented May 3, 2019

Field "id_lang" in emailsubscription table
Issue: PrestaShop/PrestaShop#13453

Codencode added some commits Apr 17, 2019

@marionf

This comment has been minimized.

Copy link

commented May 6, 2019

Thank you @Codencode

Is it possible to add a column to display the language in the module below ?

capture d'écran_1494

This could be usefull to avoid going into databse to get this information

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Thank you @Codencode

Is it possible to add a column to display the language in the module below ?

capture d'écran_1494

This could be usefull to avoid going into databse to get this information

Thank you @Codencode

Is it possible to add a column to display the language in the module below ?

capture d'écran_1494

This could be usefull to avoid going into databse to get this information

@marionf
I have just added the code to do it.

Codencode added some commits May 6, 2019

@marionf

This comment has been minimized.

Copy link

commented May 6, 2019

@Codencode

I have the error below when I try to configure the module

capture d'écran_1495

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@Codencode

I have the error below when I try to configure the module

capture d'écran_1495

@marionf
Sorry I made a copy/paste error.
Anyway I solved it.

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@marionf
Previously I also proposed a solution to an error, if you want you can take a look here:
PrestaShop/PrestaShop#13668

Thanks!

@marionf

This comment has been minimized.

Copy link

commented May 6, 2019

@Codencode

I just found a new issue after subscribing to the newsletter with GB language

capture d'écran_1497

Previously I also proposed a solution to an error, if you want you can take a look here:
PrestaShop/PrestaShop#13668

Your PR should be reviewed by our developers before to be tested

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@marionf

I also did some tests with the GB language and I haven't found problems. However, the error should not depend on the changes I have made.
I believe that the system did not correctly evaluate the "newsletter_date_add" field when registering for the newsletter.

@marionf

This comment has been minimized.

Copy link

commented May 6, 2019

Humm, that's weird because I don't have it when I switch on master branch
Please, take a look at the screenrecord below

https://drive.google.com/file/d/15px51utBqqBFaXnhX2BkP0v9VPO3OPd7/view?usp=sharing

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Humm, that's weird because I don't have it when I switch on master branch
Please, take a look at the screenrecord below

https://drive.google.com/file/d/15px51utBqqBFaXnhX2BkP0v9VPO3OPd7/view?usp=sharing

@marionf
I solved it.

Moreover, I noticed that when the user is registring at the site and simultaneously he is registring to the newsletter, if the newsletter voucher is active (configuration NW_VOUCHER_CODE) the system does not send the email with the code. Can I make the change in this patch, or do I need to make a new pull request?

Registration
Configuration

@marionf

This comment has been minimized.

Copy link

commented May 7, 2019

@Codencode

It seems there is still an issue with GB language.
I try to register to the newsletter with GB language and the subscriber isn't displayed in the module

https://drive.google.com/file/d/1zeCY4sXOO7fsf_j6jUWQwk2f-z1fLIVu/view?usp=sharing

Moreover, I noticed that when the user is registring at the site and simultaneously he is registring to the newsletter, if the newsletter voucher is active (configuration NW_VOUCHER_CODE) the system does not send the email with the code. Can I make the change in this patch, or do I need to make a new pull request?

Could you please create another PR as it's another issue ?

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@marionf

It seems there is still an issue with GB language.
I try to register to the newsletter with GB language and the subscriber isn't displayed in the module

The list shows only the active emails. To view the last one, you need to activate the registration via the email sent to you.

Could you please create another PR as it's another issue?

Yes, can I use version 2.3.3 for the new PR?

@marionf

This comment has been minimized.

Copy link

commented May 9, 2019

Oh yes, my bad I forget that !
It seems good for me, thank you @Codencode
Let's ask for code review now @PrestaShop/prestashop-core-developers
And sorry but I don't have the answer for the version, let's ask also to @PrestaShop/prestashop-core-developers

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Oh yes, my bad I forget that !
It seems good for me, thank you @Codencode
Let's ask for code review now @PrestaShop/prestashop-core-developers
And sorry but I don't have the answer for the version, let's ask also to @PrestaShop/prestashop-core-developers

@marionf
Ok,
I asked it because this new modification requires the module to be hooked to 2 hooks, so I have to create instructions for upgrading from the previous version.

Thanks!

@jolelievre

This comment has been minimized.

Copy link

commented May 14, 2019

Hello @Codencode

I'm wondering why do you need to store the language of the user? As you can get it through the customer table, and it is more up to date.
Or do you only need it for visitors that subscribed to the newsletter without creating an account yet?

Also since this is a new feature the new version number should rather be 2.4.0 (the last digit is used for bug fixes)�

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Hello @Codencode

I'm wondering why do you need to store the language of the user? As you can get it through the customer table, and it is more up to date.
Or do you only need it for visitors that subscribed to the newsletter without creating an account yet?

It is useful to know the language of visitors registered for the newsletter and not to the site.

Also since this is a new feature the new version number should rather be 2.4.0 (the last digit is used for bug fixes)

It's true, I hadn't really thought of it.

Thank you!

Codencode added some commits May 15, 2019

@jolelievre
Copy link

left a comment

Thank you @Codencode, this now needs to be validated by QA team ;)

@jolelievre
Copy link

left a comment

Oh my bad you still need to update the config.xml file for the version as well

@jolelievre

This comment has been minimized.

Copy link

commented May 22, 2019

Also you need to target the dev branch the master branch is only used by the PrestaShop team when we will release the new module version

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Oh my bad you still need to update the config.xml file for the version as well

Yes, I just did it.

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Also you need to target the dev branch the master branch is only used by the PrestaShop team when we will release the new module version

Ok. Could you explain me how can I do it?

@jolelievre jolelievre changed the base branch from master to dev May 23, 2019

@jolelievre

This comment has been minimized.

Copy link

commented May 23, 2019

You just need to"edit" your PR and change the target branch (I did for you 😉)

@Codencode

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

You just need to"edit" your PR and change the target branch (I did for you 😉)

OK, thanks!

Can you also take a look at this request regarding the same module? #35
Thank you!

@sarahdib

This comment has been minimized.

Copy link

commented May 24, 2019

Hello @Codencode

Thank you for your contribution :)

@sarahdib sarahdib added QA approved and removed Waiting for QA labels May 24, 2019

@PierreRambaud PierreRambaud merged commit 2e3b67b into PrestaShop:dev Jul 8, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Thanks @Codencode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.