Fixed invalid method call

[Bartlomiejsz]

Description (*)

Issue spotted while trying to request invoice item details for order with discount, but without discount description. In that case Internal server error occurs

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

No steps required I believe, just make sure that existing method is being called :)

Questions or comments

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)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @Bartlomiejsz. Thank you for your contribution
Here are 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:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

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]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[bgorski]

This fixed obvious 3 incorrect calls. Marking as approved.

[ihor-sviziev]

@magento run Database Compare, Functional Tests CE, Integration Tests, Static Tests, Unit Tests, WebAPI Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[mrtuvn]

This changes would consider at first queue deploy. Changes risk low

[engcom-Alfa]

Thanks for the contribution and collaboration @Bartlomiejsz and @bgorski !

Tried reproducing the issue in 2.4-develop version before testing the given PR fix code referring PR description/ comments.
Issue is not reproducible and below is the information on this:

Pre Conditions

1. Installed a fresh 2.4-develop version

Reproducible steps

1. Created the cart rule from front end having the coupon code

2. Created a new order using front end login applying the above created coupon code.

3. Admin side: Sales > Order : Submitted the shipment and followed by Invoice

4. Viewed Invoices grid, and then clicked on the generated invoice "view"

Expected Issue as per PR description/ comments:

Internal Server Error

Actual Result

Getting a proper Invoice viewed page with all the invoice information
Below is the actual screenshot

Since there is no manual scenario to execute, based on the given one description I have tried to reproduce as mentioned above. In order to proceed forward from QA stage, require a proper steps to reproduce the issue before fix. Kindly provide me detailed steps to reproduce please!

[Bartlomiejsz]

Hi @engcom-Alfa, as the code changes graphql modules, you got to request those invoices with the items using graphql. Also, please make sure that the order has no value in discount_description field in db.

Another way to check those changes is to simply add _('Test translation'); anywhere in the code, in any controller, model etc. Using this class later will result in exception, and fixing method call by changing to __('Test translation'); will again make it work

[engcom-Alfa]

Hi @Bartlomiejsz
Thanks for the above information. Well, tried to query the graphql request as like shown in below screenshot.

Invoices with/without discount_description in the tables sales_order & sales_invoice, responding with internal server error. Responding the same before fix case, and after PR fix case as well.
Kindly provide me the exact query you are using to reproduce the issue in 2.4-develop version.
Thanks in advance!

[bgorski]

@engcom-Alfa You are using a deprecated query. Which indeed doesn't work - thanks for letting me know, I'll create a separate PR for that.
As for the issue reported here, please use the following steps:

1. As a precondition - use a Magento store in a language other than English and with a valid translation file translating the "Discount" phrase (having problems with that? See the bottom of this comment)
2. Create a cart price rule applying to all store views and all groups and with no coupon code (that's important!), no conditions and actions like 20 discount amount (effectively applying 20% off to all products in cart for everyone)
3. Register a customer and place an order in the admin panel for that customer
4. Generate an invoice for that order
5. Use the following GraphQL mutation to generate the customer token:

mutation {
  generateCustomerToken(
    email: "[email protected]"
    password: "your-password"
  ) {
    token
  }
}

6. Use the token from from the previous mutation to authorize the following query:

query {
    customer {
        orders {
            items {
                order_number
                grand_total
                status
                invoices {
                    id
                      items {
                          id
                          discounts {
                              amount {
                                value
                              }
                              label
                          }
                      }
                }
            }            
        }
    }
}

This will trigger the faulty behavior.

What's important about this is that your Magento instance will probably have the _('text here') function defined. So on an English store you may not see any error, but the phrase inside won't be translated (in this context, the _('Discount') call in Magento\SalesGraphQl\Model\Resolver\Invoice\InvoiceItems is a faulty attempt to translate the "Discount" word to the store language). If you want to be double sure about the faulty behavior, you need to edit the code of Magento\SalesGraphQl\Model\Resolver\Invoice\InvoiceItems and replace _('Discount') with _('Discount')->render(). Which would work for the intended __('Discount') function (the actual translation - please notice the double underscore instead of just one), but will throw an error for _('Discount').

[engcom-Alfa]

Thank you so much for the detailed reproducible steps @bgorski !

Manual testing scenario:

1. Executed all the given steps as per the above comment. (Note- Even after non-rendered translated "Discount" phrase, I could able to reproduce using Arabic SA language)

Before: ✖️ Getting an error for the GraphQL invoice details request.

After: ✔️ GraphQL invoice details request works as well as expected.

The actual response received is displayed below
{ "data": { "customer": { "orders": { "items": [ { "order_number": "000000001", "grand_total": 69, "status": "Complete", "invoices": [ { "id": "MQ==", "items": [ { "id": "MQ==", "discounts": [] } ] } ] }, { "order_number": "000000002", "grand_total": 34, "status": "Processing", "invoices": [] }, { "order_number": "000000003", "grand_total": 1472.6, "status": "Complete", "invoices": [ { "id": "Mg==", "items": [ { "id": "Mg==", "discounts": [ { "amount": { "value": 10 }, "label": "qqqq" } ] } ] } ] }, { "order_number": "000000004", "grand_total": 42.44, "status": "Pending", "invoices": [] }, { "order_number": "000000005", "grand_total": 66.44, "status": "Pending", "invoices": [] }, { "order_number": "000000006", "grand_total": 42.44, "status": "Processing", "invoices": [ { "id": "Mw==", "items": [ { "id": "Mw==", "discounts": [ { "amount": { "value": 7.8 }, "label": "Discount" } ] } ] } ] } ] } } } }

There is no other additional regression testing is required since it is a particular graphql request related issue.

Since it is related to Qraphql, its testing must be covered with auto-test execution. Can you please add the auto-test execution on this @Bartlomiejsz

[Bartlomiejsz]

@engcom-Alfa thank you for testing the PR
Regarding info about automated tests - I believe the PR was marked as Auto-Tests: Not Required for two reasons:

• my changes fixes really obvious mistakes done by initial code authors, so to test only my changes I would have to write something like if (function_exists('__')) {...} which makes no sense
• if on the other hand I would have to write test for whole functionality, that would mean that, to process the PR, I would have to create test for code that someone else created (with mistakes) and which was created without the tests, that as you're saying, are required

I won't be able to write test in any of those options, so if you will still believe that this is required, unfortunately this will have to be picked up by someone else...

[bgorski]

@engcom-Alfa as for tests, what we're dealing with here is a function-level fix - a replacement of a faulty function with a correct one. This has no chance of regressing and also affects no other parts of the code because it doesn't change any data, only outputs a text. If we were to create a test for it, what we would really test is a basic core logic (if the correct __('text here') function works as it should), not anything really related to the logic of this graphql call - and we would need a rather complex test for it. Which means that we would increase the complexity of the testsuite, and that's also something we have to maintain (in this case we'd have a test that can regress that tests a functionality that can't).
This is why I marked this as something that doesn't require automated tests - I believe a manual one is enough here.

[engcom-Alfa]

Thank you for the detailed information @Bartlomiejsz and @bgorski , I will process it for merging!

[engcom-Alfa]

✔️ QA Passed

Manual testing scenario:

1. Executed all the given steps as per the above comment. (Note- Even after non-rendered translated "Discount" phrase, I could able to reproduce using Arabic SA language)

Before: ✖️ Getting an error for the GraphQL invoice details request.

After: ✔️ GraphQL invoice details request works as well as expected.

The actual response received is displayed below
{ "data": { "customer": { "orders": { "items": [ { "order_number": "000000001", "grand_total": 69, "status": "Complete", "invoices": [ { "id": "MQ==", "items": [ { "id": "MQ==", "discounts": [] } ] } ] }, { "order_number": "000000002", "grand_total": 34, "status": "Processing", "invoices": [] }, { "order_number": "000000003", "grand_total": 1472.6, "status": "Complete", "invoices": [ { "id": "Mg==", "items": [ { "id": "Mg==", "discounts": [ { "amount": { "value": 10 }, "label": "qqqq" } ] } ] } ] }, { "order_number": "000000004", "grand_total": 42.44, "status": "Pending", "invoices": [] }, { "order_number": "000000005", "grand_total": 66.44, "status": "Pending", "invoices": [] }, { "order_number": "000000006", "grand_total": 42.44, "status": "Processing", "invoices": [ { "id": "Mw==", "items": [ { "id": "Mw==", "discounts": [ { "amount": { "value": 7.8 }, "label": "Discount" } ] } ] } ] } ] } } } }

There is no other additional regression testing is required since it is a particular Graphql request related issue (Above comment1 and comment2 has more info on this)

[m2-assistant]

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.