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

fix #30094 function call on array #30516

Closed
wants to merge 2 commits into from
Closed

fix #30094 function call on array #30516

wants to merge 2 commits into from

Conversation

Morgy93
Copy link
Member

@Morgy93 Morgy93 commented Oct 16, 2020

Description (*)

Fixes #30094:
Call to a member function getId() on array in module-sales/view/adminhtml/templates/order/totals/tax.phtml:62

Fixed Issues (if relevant)

  1. Fixes Call to a member function getId() on array in module-sales/view/adminhtml/templates/order/totals/tax.phtml:62 #30094

Manual testing scenarios (*)

  1. Set config tax/sales_display/full_summary to 1
  2. Open any invoice (e.g. /admin/sales/invoice/view/invoice_id/1/)

Questions or comments

Issue is introduced here 70ede03
Using the same rather "workaround" as below for $infoId: https://github.com/magento/magento2/blob/2.4.1/app/code/Magento/Sales/view/adminhtml/templates/order/totals/tax.phtml#L91

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)

Resolved issues:

  1. resolves [Issue] fix #30094 function call on array #33638: fix Call to a member function getId() on array in module-sales/view/adminhtml/templates/order/totals/tax.phtml:62 #30094 function call on array

Resolved issues:

  1. resolves [Issue] fix #30094 function call on array #33639: fix Call to a member function getId() on array in module-sales/view/adminhtml/templates/order/totals/tax.phtml:62 #30094 function call on array

Fixes #30094:
Call to a member function getId() on array in module-sales/view/adminhtml/templates/order/totals/tax.phtml:62
@Morgy93
Copy link
Member Author

Morgy93 commented Oct 16, 2020

@magento run all tests

@Morgy93
Copy link
Member Author

Morgy93 commented Oct 16, 2020

@magento run 3

@Morgy93
Copy link
Member Author

Morgy93 commented Oct 16, 2020

@magento run Functional Tests EE

Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @Morgy93. Thank you for providing the fix. Could you, please, check my suggestions below?

Thanks!

@ghost ghost assigned rogyar Oct 17, 2020
@rogyar rogyar added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Progress: needs update and removed Progress: needs update labels Oct 17, 2020
@Morgy93
Copy link
Member Author

Morgy93 commented Oct 17, 2020

@rogyar Thanks for the review! I just replied to your comment.

@Morgy93
Copy link
Member Author

Morgy93 commented Oct 17, 2020

@magento run Functional Tests EE
@magento run Functional Tests B2B

Any clue why those two fail? I doubt it's because this marginal PR change, but who knows..

@Morgy93
Copy link
Member Author

Morgy93 commented Oct 17, 2020

See.... without any changes the Functional Tests EE succeeded. After the third try..
Now onto the last one:
@magento run Functional Tests B2B

@Morgy93
Copy link
Member Author

Morgy93 commented Oct 17, 2020

@magento run Functional Tests B2B

@sidolov sidolov added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Oct 19, 2020
@rogyar
Copy link
Contributor

rogyar commented Oct 20, 2020

Hi @Morgy93. According to the definition of done all the changes must be covered with automated tests. Could I ask you to cover this fix with an MFTF test, please?

Thank you

@Morgy93
Copy link
Member Author

Morgy93 commented Oct 20, 2020

@rogyar There's already a MFTF test for that page:
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Sales/Test/Mftf/Section/AdminOrderInvoiceViewSection.xml

Correct me if I'm wrong, but it doesn't seem to need an update,

@Morgy93 Morgy93 mentioned this pull request Oct 23, 2020
4 tasks
@rogyar
Copy link
Contributor

rogyar commented Oct 24, 2020

Hi @Morgy93. We don't have a test that reproduces the steps described in the original issue and checks that we don't have the error anymore. So, we need to add a new test.

Basically, the new test will do the following

  • Go to Store -> Configuration -> Sales -> Tax -> Orders, Invoices, Credit Memos Display Settings -> Display Full Tax Summary and change to YES
  • Go to Sales -> Orders -> Open Order -> Invoice Tab -> Open Invoice -> Make sure that there's no "Call to a member function" is not present in the page source.

Hope it will help

@Morgy93
Copy link
Member Author

Morgy93 commented Jul 30, 2021

@magento create issue

Actually there's an issue already: #30094 (comment)

It just got closed as "non-reproducible".
For the history and other contributors it would probably be better to use that one instead.

@hostep
Copy link
Contributor

hostep commented Aug 11, 2021

Well, what do you know, Magento just released a similar fix in MC-42332: part 1, part 2, it's great to see how the community maintainers and the internal Magento/Adobe core team are communicating with each other.

@mrtuvn
Copy link
Contributor

mrtuvn commented Aug 12, 2021

so basically we have the fixes landed to 2.4-develop right!? So wild. Never imagine another fix come from another places

@mrtuvn
Copy link
Contributor

mrtuvn commented Aug 12, 2021

So this fix seem redundant! But thanks you for your working efforts @Morgy93 at this

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @Morgy93,
Looks like the issue was fixed in the 2.4-develop branch in d648bdc and 3ae814c. Could you confirm that?

@Morgy93
Copy link
Member Author

Morgy93 commented Aug 13, 2021

This is ridiculous!

We're waiting for almost a year to have this issue fixed, provided MULTIPLE valid fixes / PR and it involved many official staff who all refused to apply them.
Now we presented a 100% reproducible way and SUDDENLY the official staff fixed the error on its own - silently.
Without a SINGLE WORD to the community.

Big thanks to every single one of you who contributed here! Closed~

@Morgy93 Morgy93 closed this Aug 13, 2021
@m2-assistant
Copy link

m2-assistant bot commented Aug 13, 2021

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

@hostep
Copy link
Contributor

hostep commented Aug 13, 2021

@sidolov: maybe you can communicate with Adobe's internal Magento developers to first check github for open PR's when they are presented an issue they have to resolve. That could save some frustration from the community and might speed things up for accepting/merging PR's here.

/cc @o-dubovyk

@sidolov
Copy link
Contributor

sidolov commented Aug 16, 2021

@Morgy93 I apologize for the situation that happened here! Usually, we have a copy of every issue in our backlog for internal teams and developers have to check for open public pull requests first. In this case, the same issue was reported and fixed by the internal developer, that's why identical fixes came from different sources. I'm pretty sure, anyone didn't mean to copy your fix and deliver it as a part of the core code. We will try to adjust the process to avoid such cases!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix Component: Sales Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet