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

[Invoices] Display the shipping category. #11805

Conversation

abdellani
Copy link
Member

@abdellani abdellani commented Nov 13, 2023

What? Why?

What should we test?

  • create an order.
  • generate at least one invoice
  • check if you can see the right shipping category

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@abdellani abdellani force-pushed the fix-shipping-method-category-on-invoices-v3 branch from 69c57ef to 427d0f7 Compare November 13, 2023 09:25
@abdellani abdellani marked this pull request as ready for review November 13, 2023 09:25
@abdellani abdellani force-pushed the fix-shipping-method-category-on-invoices-v3 branch 2 times, most recently from a4b2819 to 830d1d0 Compare November 28, 2023 15:58
%strong= "#{t(:shipping)} "
= "( #{t(:invoice_shipping_type)} #{raw(@order.shipping_method.name)} )"
%strong= "#{@order.shipping_method.category} : "
= "(#{raw(@order.shipping_method.name)})"
Copy link
Member

Choose a reason for hiding this comment

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

Does this have to be raw? I don't want enterprises to inject code via the shipping method name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for raw here.

@dacook dacook self-requested a review November 30, 2023 23:52
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good, could we just remove the raw in the template ?

%strong= "#{t(:shipping)} "
= "( #{t(:invoice_shipping_type)} #{raw(@order.shipping_method.name)} )"
%strong= "#{@order.shipping_method.category} : "
= "(#{raw(@order.shipping_method.name)})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for raw here.

@dacook dacook removed their request for review November 30, 2023 23:53
@abdellani abdellani force-pushed the fix-shipping-method-category-on-invoices-v3 branch from 830d1d0 to e449ef0 Compare December 11, 2023 07:33
@abdellani abdellani requested a review from rioug December 11, 2023 07:40
@RachL RachL self-assigned this Dec 14, 2023
@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Dec 14, 2023
@RachL
Copy link
Contributor

RachL commented Dec 14, 2023

@abdellani this is perfect. A tiny comment: can we either remove the ":" or remove the brackets? It's a bit weird to have both, but otherwise all good, ready to merge here 💪

image

@RachL RachL added feedback-needed and removed pr-staged-fr staging.coopcircuits.fr labels Dec 14, 2023
@abdellani
Copy link
Member Author

ok @RachL
I'll remove the column ':' :)

@abdellani
Copy link
Member Author

@RachL done

@RachL
Copy link
Contributor

RachL commented Dec 14, 2023

@abdellani thanks! It looks like we need to rebase right?

@abdellani abdellani force-pushed the fix-shipping-method-category-on-invoices-v3 branch from c65c0f2 to ab85adf Compare December 14, 2023 15:36
@abdellani abdellani force-pushed the fix-shipping-method-category-on-invoices-v3 branch from ab85adf to 2b1d792 Compare December 14, 2023 15:43
@abdellani
Copy link
Member Author

@RachL
yes, that's correct.
It's ready now

@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed feedback-needed pr-staged-fr staging.coopcircuits.fr labels Dec 14, 2023
@RachL
Copy link
Contributor

RachL commented Dec 14, 2023

Perfect, merging!

@RachL RachL merged commit 97985db into openfoodfoundation:master Dec 14, 2023
52 checks passed
@RachL RachL added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Invoices] Change the display of the shipping method name
4 participants