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

Remove a field from the employee creation form #15798

Merged
merged 6 commits into from Oct 16, 2019

Conversation

@matthieu-rolland
Copy link
Contributor

matthieu-rolland commented Oct 2, 2019

Questions Answers
Branch? develop
Description? Removes a field from the employee form
Type? improvement
Category? BO
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes #15792
How to test? Check that the field is not in the form anymore.

This change is Reviewable

@matthieu-rolland matthieu-rolland requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 2, 2019
@matthieu-rolland matthieu-rolland added WIP and removed WIP labels Oct 2, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:optin-employee branch from 4f80da9 to c6437d3 Oct 4, 2019
@matthieu-rolland matthieu-rolland added the WIP label Oct 4, 2019
Copy link
Contributor

PierreRambaud left a comment

You removed id_last_order instead of adding the optin field :D

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:optin-employee branch from c6437d3 to 0209970 Oct 4, 2019
@PierreRambaud PierreRambaud dismissed their stale review Oct 4, 2019

Didn't see the WIP label -_-'

@matthieu-rolland matthieu-rolland removed the WIP label Oct 4, 2019
Copy link
Contributor

jolelievre left a comment

@matthieu-rolland Some conflicts on the sql file, but the rest is legit

Copy link
Contributor

PierreRambaud left a comment

Reviewed 12 of 14 files at r1, 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matthieu-rolland, @Progi1984, and @sarahdib)


install-dev/upgrade/sql/1.7.7.0.sql, line 17 at r2 (raw file):

Previously, Progi1984 (Progi1984) wrote…

But if we apply the patch of the PR, no value can't be saved.

We maybe don't want, but if modules does we must keep the behavior :)

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:optin-employee branch from 0209970 to 4292e16 Oct 15, 2019
@matks
matks approved these changes Oct 15, 2019
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Oct 15, 2019

@sarahdib FYI, this PR doesn't remove the optin column in database, and it doesn't change the existing values inside it.

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Oct 15, 2019
@sarahdib sarahdib added this to the 1.7.7.0 milestone Oct 15, 2019
Copy link
Contributor Author

matthieu-rolland left a comment

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @PierreRambaud and @sarahdib)


install-dev/upgrade/sql/1.7.7.0.sql, line 17 at r2 (raw file):

Previously, PierreRambaud (GoT) wrote…

We maybe don't want, but if modules does we must keep the behavior :)

Done.

@matthieu-rolland matthieu-rolland merged commit a2a7cca into PrestaShop:develop Oct 16, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 1 file, 1 discussion left (PierreRambaud, sarahdib)
Details
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.