Update sum calculation in order_cycle_customer_totals spec

[pacodelaluna]

What? Why?

• Closes #13270

Initial objective was to fix a warning, but it was not possible to reproduce the issue.
Then, we decide to update the sum calculation to be consistent with previous changes.

What should we test?

It is a refactoring, unit tests are already provided for this change.

Release notes

• Update sum calculation in reports

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

[pacodelaluna]

I unassigned the PR by mistake, but I cannot take it back, I don't have the permission.

[pacodelaluna]

I am having a concern with my changes actually.
While testing the results in console, I remarked this:

irb(main):041> Spree::Order.order(created_at: :desc).limit(100).sum(&:total).to_i
  Spree::Order Load (30.2ms)  SELECT "spree_orders".* FROM "spree_orders" ORDER BY "spree_orders"."created_at" DESC LIMIT $1  [["LIMIT", 100]]
=> 0
irb(main):036> Spree::Order.order(created_at: :desc).limit(100).map(&:total).sum(&:to_f)
  Spree::Order Load (31.9ms)  SELECT "spree_orders".* FROM "spree_orders" ORDER BY "spree_orders"."created_at" DESC LIMIT $1  [["LIMIT", 100]]
=> 0.0
irb(main):038> Spree::Order.order(created_at: :desc).limit(100).map(&:total).inspect
  Spree::Order Load (34.4ms)  SELECT "spree_orders".* FROM "spree_orders" ORDER BY "spree_orders"."created_at" DESC LIMIT $1  [["LIMIT", 100]]
=> "[0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]"

I think it would be more efficient for memory to use pluck instead of map, isn't it?
We are not sure that the list passed to the method is always a list, it could be an ActiveRecord relation or scope, right?
And Ruby lists are actually implementing pluck method in Rails (the famous Rails Way I guess :). It will be useful in our case.

irb(main):043> Spree::Order.order(created_at: :desc).limit(100).pluck(:item_total).sum(&:to_f)
  Spree::Order Pluck (39.1ms)  SELECT "spree_orders"."item_total" FROM "spree_orders" ORDER BY "spree_orders"."created_at" DESC LIMIT $1  [["LIMIT", 100]]
=> 0.0
irb(main):044> Spree::Order.order(created_at: :desc).limit(100).to_a.pluck(:item_total).sum(&:to_f)
  Spree::Order Load (35.4ms)  SELECT "spree_orders".* FROM "spree_orders" ORDER BY "spree_orders"."created_at" DESC LIMIT $1  [["LIMIT", 100]]
=> 0.0

So I will use pluck as it provides better performance when the DB request is executed inside this class.

[pacodelaluna]

Actually, it is a bit more tricky than this, LineItems object is already a map, and the request on the mapped objects can be methods, not just field, so pluck is not working in this case.

[pacodelaluna]

I have started refactoring the sum calculation for the whole reporting part. It makes the scope wider than the initial issue, but I thought it was the occasion to do it. Please tell me if I should reduce it.
By the way, I am still having 5 Rubocop warnings regarding Cyclomatic Complexity, these issues were already present, the only solution I am seeing is to introduce other methods to split values list generation, but I am not sure it will improve code readability. Please tell me if you think I should still address these issues in this PR.

[pacodelaluna]

The safe operator is used here, but maybe to replace it with .to_a will make the code more readable and the result more accurate (0 instead of nil).

[pacodelaluna]

I am assuming that the PR will be squashed before the merge, but please tell me if you have an other approach to keep the git tree clean.

[rioug]

By the way, I am still having 5 Rubocop warnings regarding Cyclomatic Complexity, these issues were already present,

I updated the rubocop TODO file so the build should green now.
FYI you can do so with this command :

bundle exec rubocop --regenerate-todo --no-auto-gen-timestamp

[rioug]

I am assuming that the PR will be squashed before the merge, but please tell me if you have an other approach to keep the git tree clean.

We don't squash PR on merge, we prefer to keep the git history. Usually it's better to clean up your git history before submitting a PR, see https://github.com/openfoodfoundation/openfoodnetwork/wiki/Making-a-great-pull-request#keep-your-commit-history-clean-tidy-and-up-to-date
But note, don't change your git history once the PR has been reviewed, otherwise it gets too confusing.

Thanks for you help, the PR looks good. 👍

[rioug]

Typo ? Will be fixed in the next commit.

[mkllnk]

Looking good!

[mkllnk]

I think that it's still a good idea to do a sanity check on reports in staging.

[pacodelaluna]

I have checked various reports on FR staging and I got the reports:

• https://staging.coopcircuits.fr/admin/reports/payments/payments_by_payment_type
• https://staging.coopcircuits.fr/admin/reports/packing/customer
• https://staging.coopcircuits.fr/admin/reports/revenues_by_hub

Now about values, I checked especially the [Revenues By Hub](report https://staging.coopcircuits.fr/admin/reports/revenues_by_hub), from 01/05/25 to 30/06/25:

• Changes branch

- Master branch The results are identical, so it seems that these changes are not affecting the reports, as we expected. Actually, it is more a coherence test than a sanity test, but I think it is fine with our needs.

Fyi, to do the test quickly, I have just switched quickly the branches on FR staging instance, and restarted Puma each time.

[pacodelaluna]

Let's try on Payment Totals report, as where were more changes here. I used the same period.

• Changes branch

- Master branch No changes, so it looks good too. I can just see a rounding issue on the shipping cost for both cases, but it is an other issue I guess.

[rioug]

@pacodelaluna there is a conflict with master, could you rebase your branch ?

[pacodelaluna]

@rioug I pushed with force by mistake, sorry about that, old habit...
Also, I removed the commit 21a4aed as it was responsible for the conflict and it was already fixed in an other PR last week.

[pacodelaluna]

@rioug I am sorry but I have also lost your fixes about Rubocop warnings when I pushed with force last week...

I could add back the Rubocop exclusions, but I couldn't regenerate the rubocop TODO file, I am getting an error locally:

1688 files inspected, 227 offenses detected, 24 offenses autocorrectable
uninitialized constant RuboCop::Formatter::DisabledConfigFormatter::StringIO

        output_buffer = StringIO.new
                        ^^^^^^^^
/bundles/ruby/3.1.0/gems/rubocop-1.64.1/lib/rubocop/formatter/disabled_config_formatter.rb:110:in `output_cop'
/bundles/ruby/3.1.0/gems/rubocop-1.64.1/lib/rubocop/formatter/disabled_config_formatter.rb:98:in `block in output_offenses'
...

I will try to fix this up asap.

[mkllnk]

I've seen that error, too. Try running ./bin/rubocop --regenerate-todo without connecting to a rubocop server. Sometimes an IDE starts a rubocop server with a different version to our app.

[pacodelaluna]

Ok, actually I succeeded in by upgrading the Rubocop version to 1.81.6, but it was adding extra warnings. Then, I run it adding stringio in the require section of the rubocop.yml file, and it passed without error.
I have rebased with the latest master changes.
Please tell me if the PR is fine now.

[deivid-rodriguez]

Nice job 💯

I got curious and run the command on top of current master branch, and I was able to reproduce the rubocop issue. I also found that upgrading RuboCop fixes the problem.

You've workaround that successfully, so not a blocker for this PR, but I'll create a separate PR to master so that the problem does not bite other devs in the future.

[pacodelaluna]

@deivid-rodriguez Great! Actually I was wondering if we should add the extra config or update Rubocop gem. But you manage it, thanks 🙏