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

Add columns to "pay your supplier" report #13037

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chahmedejaz
Copy link
Collaborator

@chahmedejaz chahmedejaz commented Dec 12, 2024

⚠️ Please use clockify code #12476 Flower Farms

What? Why?

What should we test?

  • As mentioned in the issue

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

@chahmedejaz chahmedejaz marked this pull request as ready for review December 12, 2024 20:25
@chahmedejaz chahmedejaz added the user facing changes Thes pull requests affect the user experience label Dec 12, 2024
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 👍 Note for next time, it's better to add the specs in the same commit as the changes you make. It makes reviewing easier as we can look at the changes and specs on the same page.

supplier(line_items).charges_sales_tax ? I18n.t(:yes) : I18n.t(:no)
supplier(line_items).charges_sales_tax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the specs are passing, I assume the boolean are auto-magically translated to Yes/No ?

Copy link
Member

Choose a reason for hiding this comment

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

I was curious too, so I found out the answer here: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/lib/reporting/report_row_builder.rb#L120-L122

I've been interested to try this gem for this sort of thing: https://github.com/RST-J/human_attribute_values
It would allow translators to choose a more specific description, which might be better in some circumstances.

But I think handling it this way is quite sufficient and much simpler, until someone says we need a better solution.

@@ -38,8 +38,11 @@ def suppliers_adjustments(line_item, adjustment_type = 'EnterpriseFee')
end

def adjustments_by_type(line_item, type, included: false)
is_tax = type == :tax
return 0.0 if is_tax && !supplier(line_item).charges_sales_tax
Copy link
Member

Choose a reason for hiding this comment

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

It seems strange that the report needs to check if charges_sales_tax. I thought that the application logic should be able to work this out. I see that Spree::TaxRate checks for this in match, but I don't think that helps here.

Oh well, it turns out it's quite common to refer to this directly in other reports, all good 👍

tax = total_tax.call(line_item)

total_price + total_fees + total_fees_tax + tax
total_excl_vat.call(line_item) + total_tax.call(line_item)
Copy link
Member

Choose a reason for hiding this comment

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

Nice to see some of the logic ends up simpler! 😎

supplier(line_items).charges_sales_tax ? I18n.t(:yes) : I18n.t(:no)
supplier(line_items).charges_sales_tax
Copy link
Member

Choose a reason for hiding this comment

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

I was curious too, so I found out the answer here: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/lib/reporting/report_row_builder.rb#L120-L122

I've been interested to try this gem for this sort of thing: https://github.com/RST-J/human_attribute_values
It would allow translators to choose a more specific description, which might be better in some circumstances.

But I think handling it this way is quite sufficient and much simpler, until someone says we need a better solution.

@chahmedejaz
Copy link
Collaborator Author

Note for next time, it's better to add the specs in the same commit as the changes you make. It makes reviewing easier as we can look at the changes and specs on the same page.

Sure, @rioug will take care of it from now onwards. Thanks 🙂

@chahmedejaz
Copy link
Collaborator Author

chahmedejaz commented Dec 16, 2024

I've been interested to try this gem for this sort of thing: https://github.com/RST-J/human_attribute_values
It would allow translators to choose a more specific description, which might be better in some circumstances.

Thanks for introducing this gem, David. Yes, it seems more flexible indeed. Maybe in future we may find a usecase to add this 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Test Ready 🧪
Development

Successfully merging this pull request may close these issues.

Add columns to "pay your supplier" report
3 participants