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 transport cost related issues #1082

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

twothreenine
Copy link
Contributor

@twothreenine twothreenine commented Jan 27, 2025

Bug: When creating a foodcoop transaction + finance link when settling an order, the group sum without transport costs was used for the foodcoop transaction, so whenever transport costs were involved, unbalanced finance links were created.

This commit changes:

  • Include transport costs when calculating the group sum of an order.
    This value is displayed (among other places) in the balancing summary box. I chose not to include transport costs in groups_without_markup though, since that would be more complicated and also because transport costs are some kind of "extra costs."
  • When creating a foodcoop transaction when settling an order, use the exact inverted sum of the group transactions to prevent rounding issues.
  • Display transport costs above The following were not ordered to avoid confusion.
Resolved by c7fc475: checks fail

The checks fail with the following note -- I didn't run them locally, so I don't know whether they'll pass.

An error occurred while installing ruby-filemagic (0.7.3), and Bundler cannot
continue.

In Gemfile:
  foodsoft_documents was resolved to 0.0.1, which depends on
    ruby-filemagic

@twothreenine twothreenine force-pushed the fix/include-transport-costs branch from 4205ae6 to d843be7 Compare February 7, 2025 02:28
- Include transport costs when calculating the group sum of an order
- When creating a foodcoop transaction, use the exact inverted sum of the group transactions to prevent rounding issues
- Display transport costs above "The following were not ordered" to avoid confusion
@twothreenine twothreenine force-pushed the fix/include-transport-costs branch from d843be7 to 5f03182 Compare February 7, 2025 02:35
@twothreenine
Copy link
Contributor Author

I rebased it. Ready to review @yksflip @lentschi

@mjavurek Perhaps you also want to take a look at this since you also worked on transactions.

@RayOei
Copy link

RayOei commented Mar 11, 2025

Is this the PR which (also) solves all failing tests on recent PR's?

@twothreenine
Copy link
Contributor Author

Is this the PR which (also) solves all failing tests on recent PR's?

No, that has been fixed by c7fc475. This is just the PR where I noticed it. I subsequently reported it and rebased this PR after it was fixed.

@RayOei
Copy link

RayOei commented Mar 11, 2025

Ah... thanks!

Never mind... it helps when you pull from the correct repo. I thought I was on the latest master but I was not in the correct local repo which wasn't updated/synced... 😊

@lentschi
Copy link
Contributor

lentschi commented Mar 14, 2025

I rebased it. Ready to review

I'd rather wait for #1073 to be merged (since this is bound to happen any minute now :) ) and then review the rebased version of your PR. Sorry for not reviewing/merging it earlier though, I've missed the GitHub notification.

Is that okay for you, @twothreenine, or do you think this to be something very urgent that needs to be in the upcoming 4.9.x release? (Is it an old bug or something new?)

@twothreenine
Copy link
Contributor Author

twothreenine commented Mar 14, 2025

While it isn't fixed, unbalanced financial links will be created when there are transport costs (with the feature I introduced in #1064). The UI issues are old. So nothing critical, but a bit annoying.
I think I had checked that there wouldn't be any merge conflicts with #1073 so I'd prefer to get this out before we commit to a year of finishing release 5.0, but it's also ok for me to delay it. @lentschi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants