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

Bug fix for specific product combination cases #7372

Merged
merged 3 commits into from Feb 7, 2017

Conversation

@djbuch
Copy link
Contributor

commented Jan 19, 2017

Questions Answers
Branch? develop
Description? In the case where you have 2 or more attribute groups on a product, say group A and group B, having respectively the following attributes 10, 11, 12 and 20, 21, 22.
The product having the following combinations :
- 10 & 20
- 10 & 21
- 11 & 22

It is impossible to select the 11 attribute, it will always come back to the 10. This PR fixes this bug.
Type? bug fix / improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Create a product as below, and you will see

Important guidelines


This change is Reviewable

Code Utopia added 3 commits Jan 19, 2017
Code Utopia
Wash attributes depending on group order
This is in order to not display the unexistant combinations
Code Utopia
@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2017

Hi!

Your pull request description seems to be incomplete or malformed:

  • The type should be one of: new feature, improvement, bug fix, refacto or critical.

Would you mind completing the contribution table ? This would help us understand how interesting your contribution is.

Thank you!

(note: this is an automated message, but answering it will reach a real human )

@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2017

Hi!

These(s) commit(s) name(s) seems to be incomplete or malformed, regarding our guidelines:








Malformed commits
Wash attributes depending on group order

This is in order to not display the unexistant combinations

Find the best possible combination
Find the best possible

A valid commit name can be, for instance:

BO: Shows company in BO search if B2B is enabled

Would you mind to amend your commits' names?

To do this, open a command line window and use git commit --amend for the commit's name. See GitHub's help page for more information.

Note: this must be done via the command line: you can't do this just by changing the title of the pull-request from the GitHub interface! :)

example

Thank you!

(note: this is an automated message, but answering it will reach a real human )

@antoin-m

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2017

Thank you @djbuch,

Could you please rename your commits so that they follow our guidelines ?

@vincentbz What do you think about this solution? The 1.6 behavior was to display all the different possibilities with an error message on the unavaible ones.

@djbuch

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

@xBorderie

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

@djbuch Seems like you did not use Git through the command line, but directly changed the files from the GitHub form interface. Has this code been tested? Are you sure there are not typos?

@antoin-m Can you amend and squash the commits in this situation?

@djbuch

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2017

@antoin-m

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

@xBorderie I could amend and squash but I'd rather have a review from @vincentbz first

@vincentbz vincentbz added the PM ✔️ label Feb 6, 2017

@vincentbz

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

Hi @djbuch

It changes the behaviour we wanted to have (as in 1.6) but it works and it could be more clear as only the available combinations can be displayed. Thanks !

@vincentbz vincentbz added the QA ✔️ label Feb 6, 2017

@aleeks

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

Ok, let's go to squash

@vincentbz

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

I prefer table tennis

@djbuch

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2017

@vincentbz

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

The one on 1.6 is not working in every case ?
But yours will be less confusing for people :)

@antoin-m antoin-m merged commit 5a22283 into PrestaShop:develop Feb 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@antoin-m

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

Thank you @djbuch ! 🚀

@Shudrum Shudrum changed the title FO: Bug fix for specific product combination cases Bug fix for specific product combination cases Feb 27, 2017

@eternoendless eternoendless added this to the 1.7.1.0 milestone May 13, 2019

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