#11209 Wishlist Add grouped product Error #26258
Conversation
|
Hi @MaxRomanov4669. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
rogyar
added
the
Auto-Tests: Covered
There was a problem hiding this comment.
Hi @MaxRomanov4669.
Thank you for your collaboration. Also, please, take a look at the failing static tests. It looks like there's one missing dependency in the composer.json
| $diff = array_diff_key($options1, $options2); | ||
|
|
||
| if (!$diff) { | ||
| foreach ($options1 as $key => $val) { |
There was a problem hiding this comment.
Please, use something like
foreach(array_keys($options) as $key) { ...To avoid unused variables declaration
| * @return array | ||
| */ | ||
| public function beforeCompareOptions( | ||
| WishlistItem $subject, |
There was a problem hiding this comment.
You may use @SuppressWarnings(PHPMD.UnusedFormalParameter) in case of this method if the $subject variable is unused.
|
|
||
| $superGroupObj = new \Magento\Framework\DataObject($superGroup); | ||
|
|
||
| $this->productMock->expects($this->once())->method('getId')->willReturn(34); |
There was a problem hiding this comment.
Please, assign repetitive values (34 and others) to a separate variable. It will make the code easier to maintain in the future
|
Hi @MaxRomanov4669. Thank you for the fixes. Could I also ask you to declare strict types and use types hinting for method parameters and return types? See the following plugin as an example. Thank you. |
|
Hi @rogyar I added code fixes but as I see there are new tests for Magento EE. Can you please review these tests? Thank you. |
|
Hi @MaxRomanov4669. The test execution environment is under maintenance at the moment. That's why we might see some failing tests that are not related to our changes somehow. |
|
Hi @rogyar, thank you for the review. |
ghost
added
Progress: testing in progress
and removed
Progress: ready for testing
labels
|
✔️ QA passed |
ghost
added
Progress: extended testing
and removed
Progress: testing in progress
labels
|
Hi @MaxRomanov4669, thank you for your contribution! |
Description (*)
Fixed Wishlist logic for grouped products when add same simple product
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)