NEXT-10868 - Use order currency if defined to display line items, default to context currency

[Schrank]

Closes #3575

Untested and now time to follow up currently.

Thanks @MelvinAchterhuis for the file and the fix

1. Why is this change necessary?

#3575

2. What does this change do, exactly?

Use the currency from the order not the session

3. Describe each step to reproduce the issue or behaviour.

See #3575

4. Please link to the relevant issues (if any).

#3575

5. Checklist

• I have rebased my changes to remove merge conflicts
• I have written tests and verified that they fail without my change
• I have created a changelog file with all necessary information about my changes
• I have written or adjusted the documentation according to my changes
• This change has comments for package types, values, functions, and non-obvious lines of code
• I have read the contribution requirements and fulfil them.

[MelvinAchterhuis]

@Schrank , This component is used everywhere for line items, so if an order exists we should use the order iso code, otherwise context

Should be something like this I think (untested)
|currency(order.currency.isoCode ?? context.currency.isoCode)

[Schrank]

@LarsKemper should be complete now.

@MelvinAchterhuis if you are bored, have a second view! :-)

[OliverSkroblin]

Hey @Schrank,

I would prefer when we handle this as a "template-depdency" and the currency to use will be provided from the outer one.

{% sw_include '..../line-item/element/total-price.html.twig' with {
    lineItem: lineItem,
    currency: order.currency ?? context.currency
} %}

I bet this will be not the place where the price is included, i guess you have to pass through all templates, starting where the order variable is started first

[Schrank]

Good idea!

[ssltg]

In my opinion it would be better to pass the currency from the template where we explicitly want the display to change (Storefront/Resources/views/storefront/page/account/order-history/order-detail-list.html.twig) and then only pass the currency variable through. Here we have many call on possibly not set variables, which we like to reduce.

Storefront/Resources/views/storefront/page/account/order-history/order-detail-list.html.twig

{% block page_account_order_item_detail_line_item %}
    {% sw_include '@Storefront/storefront/component/line-item/line-item.html.twig' with {
            displayMode: 'order',
            showRemoveButton: false,
           currencyCode: lineItem.order.currency.isoCode 
     } %}
{% endblock %}

all other includes:

{% sw_include '@Storefront/storefront/component/....' with {
        currencyCode: currencyCode ?? null
 } %}

also a change to the name currencyCode would be nice to avoid conflicts and be more clear of the type of variable.

[Schrank]

Thanks for the input!

The consequence of this is, that the currency isn't available anymore, so if you want to change something and not use the code, but the ISO code or the sign you need to overwrite the template one level above.
Imho this is a disadvantage.

What is your pro argument? (Beside easier to read code and a little less complexity?)

[ssltg]

I am not sure wether we don't talk about different changes. Why would the currency not be available anymore?

The change is only to pass it from the initial template and change the name, or did I miss something?

[Schrank]

If I pass down only currency.isoCode instead of currency inside the with, then the currencyEntity is not available.

So I'm talking:

    currency: order.currency ?? context.currency
vs
    currencyCode: context.currency.isoCode ?? null

Did I misunderstand you?

Oh, I think I'm talking stupid - the currencyEntity is not available at all, is it? That means, what you say is not: Replace the currencyEntity by currencyCode, but rename the variable, so other people don't make the same mistake as I just did? 😂

So the big question: Fix the name of the variable or make the currency a currencyEntity? Any opinions on that? @ssltg @OliverSkroblin

[ssltg]

Yes, that was the intention. Just rename the variable. But to pass the whole currency entity would also be ok for me, then it could stay by that name and we have more options later.

[ssltg]

I would say: change it to the currencyEntity. After all it is about the price template and it is likely that we can make use of the currencyEntity there.

[shopware-github-importer]

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-10868

Please use this issue to track the state of your pull request.