Refactor addlinks to own class take 3 (follows #21658) #23191

amenk · 2019-06-10T07:39:24Z

Description

This is a follow up of #21658 - please refer to that one for more information.

It contains two steps

Refactoring: Extract LinkProcessing functionality
Change: Allow setting link types via DI (the original reason for this PR)

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)

m2-assistant · 2019-06-10T07:39:26Z

Hi @amenk. 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.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

amenk · 2019-06-10T09:13:45Z

@sivaschenko can you review this?

Again, I don't understand the failed tests, as the reports do not show valuable info or are not available.

sivaschenko

Thanks for updates @amenk , please see my review comments

Static tests build does not have a report indeed - relaunched it.

app/code/Magento/CatalogImportExport/Model/Import/Product/LinkProcessor.php

app/code/Magento/CatalogImportExport/Model/Import/Product.php

amenk · 2019-06-10T13:49:35Z

Wondering if https://testing-service.magento-community.engineering/reports/magento/magento2/pull/23191/79eb9096-95df-45ee-99cf-ab111e939625/26732/SampleDataFunctional/allure-report-ce/index.html#behaviors/3b800d5fe38a970fe497bf12e5c9375c/b4979f4599f0efa2/ is failing by mistake or caused by my PR ...

...for code style problem existing before and add according FIXME

amenk · 2019-06-12T20:09:56Z

All checks are green..

sivaschenko · 2019-06-13T12:22:00Z

Great, thanks @amenk . Although, it is not a serious B/C problem, additional optional constructor parameters can have a negative B/C effect in some situations. So we are required to get it approved with a responsible architect. @akaplya can you please take a look at this?

amenk · 2019-06-13T12:24:49Z

I just want to get this through before the next merge conflict appears :)

amenk · 2019-06-22T14:08:17Z

@akaplya @sivaschenko I also want to understand which kind of negative effects that can be.

sivaschenko · 2019-06-26T14:05:51Z

@amenk the example of negative effect:

Optional constructor parameter added in 2.3.3
Extension created based on 2.3.3 that is relying on the optional parameter added and i.e. change the parameter with DI arguments node
That extension is installed on Magento 2.3.1 - as there is not optional parameter - we've got a problem

amenk · 2019-06-26T16:20:18Z

Thanks... But we have no other choice to add a new class, right?

amenk · 2019-07-30T14:26:09Z

bump @sivaschenko @akaplya

Can this pleeeeeease be considered for merging?

amenk · 2019-09-05T13:42:02Z

bump @sivaschenko @akaplya

this has tag needs-update but I don't know which update you are expecting from me?

Can this be progressed ?

amenk · 2019-09-05T13:44:37Z

@sivaschenko By the way: The idea to extract it to a new class originally came from @orlangur in #21230 (comment)

So I think it's the best way to proceed here. Or is there any other solution for the problem?

sidolov · 2019-09-23T16:18:02Z

@amenk you are right, idea with extraction is the best way here. Could you please cover changes with automated tests?

amenk · 2019-09-24T09:46:31Z

@sidolov Thanks for your response. Actually I did mostly make no changes to functionality, just the extraction behind the scenes.
The only functional change is, that the list of link types can be injected via DI. Not sure if that needs to be covered with tests, as DI is a core function.

amenk · 2020-02-27T13:11:24Z

I have a problem with CyclomaticComplexity of 11 of

I found a way by externalizing filtering ...

amenk · 2020-02-27T15:52:34Z

Static Tests fail for B2B, not for CE .. how can I fix this locally for B2B ???

https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/23191/2b6abf34e3ee6c73096af27d83615ac2/Statics/allure-report-b2b/index.html

amenk · 2020-02-28T07:53:44Z

@sidolov @sivaschenko All tests are green. can this be merged now?

magento-engcom-team · 2020-03-04T10:55:56Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7035 has been created to process this Pull Request
✳️ @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

amenk · 2020-03-05T10:19:12Z

@ihor-sviziev Thanks a lot for reviewing & accepting. What are the next steps towards a merge?

ihor-sviziev · 2020-03-05T16:09:51Z

@amenk it will go through testing and merging flow.
You can track the status on the following board https://github.com/orgs/magento/projects/5#card-22569262

Hope it will be really fast

amenk · 2020-03-05T16:31:24Z

@ihor-sviziev Okay, I think changes-requested by sivaschenko should be cleared, because I added the tests? All changes seem to be resolved. Shall I click "Dismiss review" ?

ihor-sviziev · 2020-03-05T16:51:47Z

@amenk no need, your PR already waiting to be tested

engcom-Alfa · 2020-03-18T10:26:38Z

✔️ QA Passed

…23191

m2-assistant · 2020-03-19T08:13:25Z

Hi @amenk, 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.

amenk added 2 commits June 10, 2019 09:30

Refactoring: Extract Link Processing Logic to own Class "LinkProcessor"

a269519

Import: Allow injecting own link types to LinkProcessor

095c434

magento-engcom-team added Component: CatalogImportExport Release Line: 2.3 labels Jun 10, 2019

amenk mentioned this pull request Jun 10, 2019

#18624 Refactoring: Extract addLinks to own class #21658

Closed

sivaschenko requested changes Jun 10, 2019

View reviewed changes

ghost assigned sivaschenko Jun 10, 2019

ghost added the Progress: needs update label Jun 10, 2019

Remove default values from property, as they are injected via DI

84ccd7f

orlangur self-assigned this Jun 10, 2019

amenk added 5 commits June 10, 2019 15:59

Fix initialisation of linkNameToId, unused variable, run phpcbf

3b2a4fe

Fix testProductWithLinks failing

f06e4d7

Fix CodeStyle problems

7dbdfd5

Fix one code sniffer problem; Add phpcs:disable...

39542fd

...for code style problem existing before and add according FIXME

Do not call deprecated code

4fbbcf1

orlangur removed their assignment Jun 22, 2019

magento#23191 Reduce CyclomaticComplexity of processLinkBunches

e05bb10

amenk added 2 commits February 27, 2020 17:52

magento#23191 Fix static tests for B2B edition, which checks tests

d437cec

magento#23191 Fix static tests: trailing comma in function call

5ca49ae

ihor-sviziev self-assigned this Mar 4, 2020

ihor-sviziev added Award: advanced Award: test coverage labels Mar 4, 2020

ihor-sviziev approved these changes Mar 4, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Mar 4, 2020

ihor-sviziev added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Mar 4, 2020

engcom-Alfa self-assigned this Mar 18, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Mar 18, 2020

ghost removed the Progress: testing in progress label Mar 18, 2020

engcom-Charlie self-assigned this Mar 18, 2020

ghost added Progress: extended testing Progress: accept and removed Progress: extended testing labels Mar 18, 2020

magento-engcom-team pushed a commit that referenced this pull request Mar 19, 2020

ENGCOM-7035: Refactor addlinks to own class take 3 (follows #21658) #…

c53b82b

…23191

magento-engcom-team merged commit 30c3417 into magento:2.4-develop Mar 19, 2020

Refactor addlinks to own class take 3 (follows #21658) #23191

Refactor addlinks to own class take 3 (follows #21658) #23191

Uh oh!

Conversation

amenk commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Jun 10, 2019

Uh oh!

amenk commented Jun 10, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amenk commented Jun 10, 2019

Uh oh!

amenk commented Jun 12, 2019

Uh oh!

sivaschenko commented Jun 13, 2019

Uh oh!

amenk commented Jun 13, 2019 via email

Uh oh!

amenk commented Jun 22, 2019

Uh oh!

sivaschenko commented Jun 26, 2019

Uh oh!

amenk commented Jun 26, 2019 via email

Uh oh!

amenk commented Jul 30, 2019

Uh oh!

amenk commented Sep 5, 2019

Uh oh!

amenk commented Sep 5, 2019

Uh oh!

sidolov commented Sep 23, 2019

Uh oh!

amenk commented Sep 24, 2019

Uh oh!

amenk commented Feb 27, 2020

Uh oh!

amenk commented Feb 27, 2020

Uh oh!

amenk commented Feb 28, 2020

Uh oh!

magento-engcom-team commented Mar 4, 2020

Uh oh!

amenk commented Mar 5, 2020

Uh oh!

ihor-sviziev commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amenk commented Mar 5, 2020

Uh oh!

ihor-sviziev commented Mar 5, 2020

Uh oh!

engcom-Alfa commented Mar 18, 2020

Uh oh!

m2-assistant bot commented Mar 19, 2020

Uh oh!

Uh oh!

amenk commented Jun 10, 2019 •

edited

Loading

ihor-sviziev commented Mar 5, 2020 •

edited

Loading