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.

Sign up for GitHub

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

Jump to bottom

Split ProductOptions to ProductDetails #22244

Merged
merged 12 commits into from Dec 14, 2020
Merged

Split ProductOptions to ProductDetails #22244

merged 12 commits into from Dec 14, 2020

Conversation

zuk3975
Copy link
Contributor

@zuk3975 zuk3975 commented Dec 4, 2020 •
edited

Questions Answers
Branch? develop
Description? See bellow table
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? part of 10632
How to test? Travis ✔️

DESCRIPTION

  • Split command UdpdateProductOptions to UpdateProductOptions + UpdateProductDetails. Same to related query result objects.
  • Move $tags from ProductOptions DTO to BasicInformation DTO
  • Move $active from generic ProductForEditing to ProductOptions
  • Add missing $showCondition property handling in ProductOptions
  • Adjust behat tests

This change is Reviewable

@zuk3975 zuk3975 requested a review from a team as a code owner December 4, 2020 11:29
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

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

@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Dec 4, 2020
@zuk3975 zuk3975 changed the title [WIP] Split ProductOptions to ProductDetails [product page migration] Split ProductOptions to ProductDetails [product page migration] Dec 4, 2020
@Progi1984 Progi1984 changed the title Split ProductOptions to ProductDetails [product page migration] Split ProductOptions to ProductDetails Dec 7, 2020
Progi1984
Progi1984 reviewed Dec 7, 2020
View reviewed changes
src/Core/Domain/Product/Exception/CannotUpdateProductException.php Outdated
/**
* When product details update fails
*/
const FAILED_UPDATE_DETAILS = 40;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please define const visibility

Progi1984
Progi1984 reviewed Dec 7, 2020
View reviewed changes
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

A small feedback

Progi1984
Progi1984 previously approved these changes Dec 7, 2020
View reviewed changes
Progi1984
Progi1984 previously approved these changes Dec 7, 2020
View reviewed changes
jolelievre
jolelievre reviewed Dec 7, 2020
View reviewed changes
src/Adapter/Product/Validate/ProductValidator.php Show resolved Hide resolved
tests/Integration/Behaviour/Features/Scenario/Product/update_details.feature
| reference | |
When I update product "product2" details with following values:
| mpn | this is more than forty characters long string |
Then I should get error that product mpn is invalid
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add invalid tests for each reference which had specific format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've added only mpn because it doesn't have Value object, all other references have and are tested by units

Progi1984
Progi1984 previously approved these changes Dec 8, 2020
View reviewed changes
@matks matks added the migration symfony migration project label Dec 8, 2020
@matks matks added this to the 1.7.8.0 milestone Dec 8, 2020
matks
matks previously approved these changes Dec 9, 2020
View reviewed changes
Progi1984
Progi1984 previously approved these changes Dec 10, 2020
View reviewed changes
@zuk3975
Copy link
Contributor Author

zuk3975 commented Dec 10, 2020

rr build

@zuk3975 zuk3975 closed this Dec 10, 2020
@zuk3975 zuk3975 reopened this Dec 10, 2020
@matks matks merged commit 747d41d into PrestaShop:develop Dec 14, 2020
@matks matks deleted the m/product/details branch December 14, 2020 10:18
@matks
Copy link
Contributor

matks commented Dec 14, 2020

Thank you @zuk3975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Progi1984 Progi1984 Progi1984 approved these changes

@matks matks matks approved these changes

@jolelievre jolelievre Awaiting requested review from jolelievre

Assignees
No one assigned
Labels
develop Branch migration symfony migration project Refactoring Type: Refactoring
Projects
None yet
Milestone
1.7.8.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zuk3975 @prestonBot @matks @Progi1984 @jolelievre