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

Change the APE field validation to match all formats #639

Merged
merged 3 commits into from Nov 2, 2023

Conversation

alexandrebak42
Copy link
Contributor

@alexandrebak42 alexandrebak42 commented Oct 11, 2023

Questions Answers
Description? This PR is linked to PR 34242. It just update the PREFIX_customer.ape field from varchar(5) to varchar(6) to match all french APE code formats. I hope I haven't forgotten anything.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes for PR 34242, Fixes for issue 34216
Sponsor company coquille.fr
How to test? The ape field in PREFIX_customer table should be varchar(6) after upgrade.

Copy link
Member

@boherm boherm left a comment

Choose a reason for hiding this comment

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

Hello @alexandrebak42! Thanks for your contribution.
As you target develop branch (and so the 9.0.0 version of PrestaShop) in your PrestaShop/PrestaShop#34217, you have to edit the sql/9.0.0.sql file instead of 8.1.3.sql.

@alexandrebak42
Copy link
Contributor Author

Oh ok sorry, this is my first PR on an open-source repository ! If I wanted the changes to take effect on 8.1.3 I should have done my PR on 8.1.X ?

@boherm
Copy link
Member

boherm commented Oct 11, 2023

Oh ok sorry, this is my first PR on an open-source repository !

No problem! 😉

If I wanted the changes to take effect on 8.1.3 I should have done my PR on 8.1.X ?

Yes, your PR must target the 8.1.x branch in that case.
By the way, it's preferable in your case I think 🤔 And in that case, this PR is good!

@alexandrebak42
Copy link
Contributor Author

Yes, your PR must target the 8.1.x branch in that case. By the way, it's preferable in your case I think 🤔 And in that case, this PR is good!

Nice, I created a new PR on 8.1.x here

mflasquin
mflasquin previously approved these changes Oct 11, 2023
M0rgan01
M0rgan01 previously approved these changes Oct 24, 2023
@alexandrebak42
Copy link
Contributor Author

I just resolved conflicts

@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 2, 2023

@alexandrebak42 Good job, works as it should. :-)

BEFORE

AFTER

@Hlavtox Hlavtox added this to the 4.16.5 milestone Nov 2, 2023
@Hlavtox Hlavtox merged commit 083cb5d into PrestaShop:dev Nov 2, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
6 participants