Fix #30073 - Store specific text swatch attribute option label with v… #30114

Bartlomiejsz · 2020-09-19T08:40:28Z

…alue 0 (zero) can't be saved and automatically replaced with Admin Label value of option

Description (*)

The issue is fixed specifically by https://github.com/magento/magento2/pull/30114/files#diff-e82f73165c79d2422b9b50632a4ff4feL355 - rest of modifications in the class are just cleanup

Related Pull Requests

Fixed Issues (if relevant)

Fixes Store specific text swatch attribute option label with value 0 (zero) can't be saved and automatically replaced with Admin Label value of option #30073

Manual testing scenarios (*)

Please check fixed issue for testing scenario

Questions or comments

I believe unit test should be enough for such type of change

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

… with value 0 (zero) can't be saved and automatically replaced with Admin Label value of option

m2-assistant · 2020-09-19T08:40:31Z

Hi @Bartlomiejsz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

Bartlomiejsz · 2020-09-19T08:40:48Z

@magento run all tests

Bartlomiejsz · 2020-09-19T12:34:18Z

@magento run Integration Tests

rogyar · 2020-09-19T15:09:22Z

Hi @Bartlomiejsz. Great work, thank you!
I don't see the failing integration tests, only the skipped ones. So, I believe we don't have an issue there.

Pull Request state was updated. Re-review required.

magento-engcom-team · 2020-09-19T15:10:44Z

Hi @rogyar, thank you for the review.
ENGCOM-8234 has been created to process this Pull Request

Stepa4man

Hi @Bartlomiejsz
Thanks for your contribution

Could you please take a look at my comments?

Stepa4man · 2020-09-21T06:31:23Z

app/code/Magento/Swatches/Model/Plugin/EavAttribute.php

-                if (isset($optionsArray['delete']) && isset($optionsArray['delete'][$optionId])
-                    && $optionsArray['delete'][$optionId] == 1
-                ) {
+                if (isset($optionsArray['delete'][$optionId]) && $optionsArray['delete'][$optionId] == 1) {


Could you please simplify this nested ifs, handling the negative case first and use a strict comparison operator?

Hi @Stepa4man, done

Stepa4man · 2020-09-21T06:35:30Z

app/code/Magento/Swatches/Model/Plugin/EavAttribute.php

+        if (strpos((string)$optionId, self::BASE_OPTION_TITLE) === 0 ||
+            strpos((string)$optionId, self::API_OPTION_PREFIX) === 0) {


Not clear what is checked here from the first glance. Can I ask you to move these conditions to another private method and name it so it displays what exactly it checks.

To be honest I'm not sure what is purpose of this check, and I'm not author of this part. I only replaced substr with strpos since it made more sense in such comparison in my opinion...

Ok, I think we can live with this=)

Stepa4man · 2020-09-21T07:07:33Z

@magento give me test instance

magento-deployment-service · 2020-09-21T07:07:35Z

Hi @Stepa4man. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service · 2020-09-21T07:12:46Z

Hi @Stepa4man, here is your Magento Instance: https://d20d7c5c6b2ce0957aacf88809bb4839.instances.magento-community.engineering
Admin access: https://d20d7c5c6b2ce0957aacf88809bb4839.instances.magento-community.engineering/admin_79da
Login: 5d6b4a37
Password: beb696f80e44

magento-engcom-team · 2020-11-11T12:22:43Z

Hi @gabrieldagama, thank you for the review.
ENGCOM-8234 has been created to process this Pull Request

magento-engcom-team · 2020-11-11T15:08:48Z

Hi @Stepa4man, thank you for the review.
ENGCOM-8234 has been created to process this Pull Request

engcom-Alfa · 2020-11-12T07:13:17Z

✔️ QA Passed

Manual testing scenario:

Create multistore Magento instance;
Admin -> Stores -> Attribute -> Product and Create text swatch attribute;
Insert in non-default store label input field 0;
Save Attribute;

Before: ✖️ Instead of 0 option store specific label is replaced with Admin label value

After: ✔️ Store-specific option label saved as inserted

… label with v… #30114

m2-assistant · 2020-11-13T13:35:07Z

Hi @Bartlomiejsz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Stepa4man · 2020-12-21T07:36:58Z

@sidolov which version will contain these commits changes?

sidolov · 2020-12-21T15:30:53Z

@Stepa4man it's included to 2.4.2 release.

Fix magento#30073 - Store specific text swatch attribute option label…

080a349

… with value 0 (zero) can't be saved and automatically replaced with Admin Label value of option

ghost added the Progress: pending review label Sep 19, 2020

magento-engcom-team added Component: Swatches Release Line: 2.4 Partner: Fast White Cat partners-contribution Pull Request is created by Magento Partner labels Sep 19, 2020

rogyar added the Award: bug fix label Sep 19, 2020

rogyar self-assigned this Sep 19, 2020

rogyar previously approved these changes Sep 19, 2020

View reviewed changes

ghost removed the Progress: pending review label Sep 19, 2020

rogyar added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Progress: pending review labels Sep 19, 2020

ghost added the Progress: ready for testing label Sep 19, 2020

ghost removed the Progress: ready for testing label Sep 19, 2020

Stepa4man requested changes Sep 21, 2020

View reviewed changes

ghost assigned Stepa4man Sep 21, 2020

ghost added Progress: needs update and removed Progress: pending review labels Sep 21, 2020

Stepa4man removed their assignment Sep 21, 2020

engcom-Echo self-assigned this Sep 21, 2020

sidolov added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Sep 21, 2020

m2-community-project bot added Progress: ready for testing and removed Progress: pending review labels Nov 11, 2020

engcom-Alfa self-assigned this Nov 11, 2020

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Nov 11, 2020

Stepa4man approved these changes Nov 11, 2020

View reviewed changes

m2-community-project bot assigned Stepa4man Nov 11, 2020

m2-community-project bot added Progress: ready for testing and removed Progress: testing in progress labels Nov 11, 2020

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Nov 12, 2020

engcom-Alfa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Nov 12, 2020

m2-community-project bot added Progress: ready for testing Progress: testing in progress Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: ready for testing Progress: extended testing labels Nov 12, 2020

gabrieldagama mentioned this pull request Nov 13, 2020

Store specific text swatch attribute option label with value 0 (zero) can't be saved and automatically replaced with Admin Label value of option #30073

Closed

magento-engcom-team pushed a commit that referenced this pull request Nov 13, 2020

ENGCOM-8234: Fix #30073 - Store specific text swatch attribute option…

c8824ee

… label with v… #30114

magento-engcom-team merged commit 43d1f59 into magento:2.4-develop Nov 13, 2020

Bartlomiejsz deleted the bugfix/30073_store_specific_text_swatch_values branch November 13, 2020 17:26

		if (strpos((string)$optionId, self::BASE_OPTION_TITLE) === 0 \|\|
		strpos((string)$optionId, self::API_OPTION_PREFIX) === 0) {

Fix #30073 - Store specific text swatch attribute option label with v… #30114

Fix #30073 - Store specific text swatch attribute option label with v… #30114

Uh oh!

Conversation

Bartlomiejsz commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Sep 19, 2020

Uh oh!

Bartlomiejsz commented Sep 19, 2020

Uh oh!

Bartlomiejsz commented Sep 19, 2020

Uh oh!

rogyar commented Sep 19, 2020

Uh oh!

magento-engcom-team commented Sep 19, 2020

Uh oh!

Stepa4man left a comment

Choose a reason for hiding this comment

Uh oh!

Stepa4man Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Bartlomiejsz Sep 27, 2020

Choose a reason for hiding this comment

Uh oh!

Stepa4man Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Bartlomiejsz Sep 27, 2020

Choose a reason for hiding this comment

Uh oh!

Stepa4man Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

Stepa4man commented Sep 21, 2020

Uh oh!

magento-deployment-service bot commented Sep 21, 2020

Uh oh!

magento-deployment-service bot commented Sep 21, 2020

Uh oh!

magento-engcom-team commented Nov 11, 2020

Uh oh!

magento-engcom-team commented Nov 11, 2020

Uh oh!

engcom-Alfa commented Nov 12, 2020

Uh oh!

m2-assistant bot commented Nov 13, 2020

Uh oh!

Stepa4man commented Dec 21, 2020

Uh oh!

sidolov commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Bartlomiejsz commented Sep 19, 2020 •

edited

Loading

sidolov commented Dec 21, 2020 •

edited

Loading