[Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master #3415

kristinalim · 2019-01-31T05:00:15Z

What? Why?

Closes Build Enterprise Fee Summary Report #2616
Relates to Add link and description to /admin/reports #2656
Relates to Create View for Fee Summary Report #2672

This would merge the transitional branch for the Enterprise Fee Summary to master.

What should we test?

Do a general test of whether this breaks the layout and that checkout is not broken.
Do a smoke test of report generation:
a. Link in the admin Reports page
b. Filters are working
c. CSV report is working
d. HTML report (below filters) is working

Release notes

Add "Enterprise Fee Summary" report available only to superadmin users.

Changelog Category: Added

Discourse thread

https://community.openfoodnetwork.org/t/report-that-isolates-various-enterprise-fees/1361

Dependencies

3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

kristinalim · 2019-01-31T05:15:03Z

@kirstenalarsen Were you able to test #3115 with the recommendations I made in the issue description before? I was the one who merged that PR in the transitional branch, so I included an updated version of that (I added Items 2 and 5) here for a more thorough round of testing by a non-dev, before everything is merged to master.

kristinalim · 2019-01-31T06:31:17Z

I moved this PR straight to "Test Ready" because these changes have already been approved in code review.

kirstenalarsen · 2019-02-01T00:54:30Z

@kristinalim I cannot stage this because the Semaphore build has failed . .

mkllnk

This pull request is too big to review thoroughly. But we reviewed all the individual parts. So it should be fine.

I'm really surprised by the huge amount of code for one report. Why do you think this report needs so much more code than others? Is it the nature of the complicated data structures?

kristinalim · 2019-02-01T06:04:29Z

@mkllnk I was surprised too, actually - I expected a lot of lines but not this much.

Although it makes sense:

The code is split quite well into responsibilities (e.g. permissions, rendering, mapping DB result columns to report columns), for better readability and testability.
We also have well formatted SQL left joins in Scope with lots of code whitespace. That class owns more than 10% of this PR in terms of added lines. (Preferred SQL left joins over eager/lazy -loading for better performance.) There is a pending issue [Enterprise Fee Summary] Explore refactoring the SQL in Scope wishlist#352 for seeing if ActiveRecord associations can be reused here.
Great test coverage, IMO. 👍

Looking at the line count, almost half of added lines are for new test files:

$ wc -l $(find . -path "./spec/validators/*" -or -path "./engines/order_management/spec/*" -or -path ./spec/features/admin/reports/enterprise_fee_summaries_spec.rb -or -path spec/controllers/spree/admin/reports/enterprise_fee_summaries_controller_spec.rb)
[...]
   1491 total

luisramos0

I think the question would be: do you see how would you do this in multiple PRs that could go live individually? I think we need to learn to do that as a team.

Otherwise 👏 specially for your very high tests/code ratio

kirstenalarsen · 2019-02-08T06:21:26Z

sooo. I am going to say that this is ok to merge to master as it is restricted to superadmin and it passes the tests above and doesn't break anything

I have found a few issues where I'm not sure what it's doing - BUT I think we are going to be so much better off to look at this with real data . . so let's get it through!
https://docs.google.com/document/d/1IxYHgzITDn0QcV-Po9sm1310cNiCdHTDx2kOGGlLrfY/edit

kirstenalarsen · 2019-02-08T06:23:16Z

Merge to master as passes tests above and is restricted to superadmin

There are some confusions, that I think need testing with real data
https://docs.google.com/document/d/1IxYHgzITDn0QcV-Po9sm1310cNiCdHTDx2kOGGlLrfY/edit

sauloperez · 2019-02-08T15:51:28Z

./spec/features/admin/product_import_spec.rb:131 failed so I retried it. Meanwhile, I'll check if it is a flaky one. I'll merge as soon as it gets 🍏

sauloperez · 2019-02-08T16:18:27Z

Bad luck. spec/features/admin/product_import_spec.rb:131 is still failing (alongside other flaky ones this time) so might be related to this PR.

EDIT doesn't fail when executed multiple times on my machine

luisramos0 · 2019-02-08T16:38:41Z

maybe rebase to get latest "flaky fixes"?

kristinalim · 2019-02-08T21:11:12Z

The product import spec failure is also an "intermittent" failure, which has been fixed here: #3430. "Intermittent" because it will pass in less than 24 hours, when it's February 10 in Melbourne time.

The other two are related to spec failures that still have open issues, but yes, better luck with a rebase.

I don't have Github permissions to modify this upstream branch though. Asking on Slack for anyone (any of you) to do this, or give me enough Github permissions. 🙂 Thanks!

Example usage: validates :start_at, date_time_string: true

Example usage: validates :related_post_ids, integer_array: true

The preferred language could change dynamically.

We should not presume what is not default for an enterprise fee object.

Make these follow the "#{SINGULAR_OBJECT}_factory.rb" convention.

Add test for more complex scenario where there is a coordinator and distributor fee for an incoming exchange, and a producer and coordinator fee for an outgoing exchange.

kristinalim · 2019-02-08T22:33:39Z

Yes! 😁 A green build. Thanks for rebasing, @luisramos0!

kirstenalarsen · 2019-02-09T01:57:46Z

So this is ready to merge I think. Yay

sauloperez · 2019-02-11T15:17:14Z

Putting the merge on hold until we get #3337 conflicts fixed.

luisramos0 · 2019-02-12T10:30:38Z

are we merging #3337 into this one and merging this one into master without staging/testing this and 3337 together?

kristinalim · 2019-02-12T10:39:18Z

@luisramos0 That confused me too, because the base branch and separation of PRs is highly dependent on the timeline...

How about we merge this tested PR into master first. Then for #3337, I will change the PR base branch of #3337 to master and rebase it, and then it can also be merged without testing again? Considering that we often rebase PRs upon master even after testing.

luisramos0 · 2019-02-12T10:43:11Z

@kristinalim you will know what's the best approach :-)
I just wanted to raise the concern so that you aware.

kristinalim · 2019-02-12T10:47:23Z

If that doesn't horrify the merger, that's the approach that I think is efficient and is still safe. 🙂 👍 I will update the other PR as soon as this is merged.

kristinalim self-assigned this Jan 31, 2019

kristinalim changed the title ~~[WIP] [Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master~~ [Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master Jan 31, 2019

daniellemoorhead assigned kirstenalarsen Feb 1, 2019

kirstenalarsen added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Feb 1, 2019

kristinalim added the pr-staged-au staging.openfoodnetwork.org.au label Feb 1, 2019

kristinalim mentioned this pull request Feb 1, 2019

3415 [Enterprise Fee Summary] Address Rubocop layout violation in Spree::Ability decorator #3424

Merged

kristinalim removed the pr-staged-au staging.openfoodnetwork.org.au label Feb 1, 2019

mkllnk approved these changes Feb 1, 2019

View reviewed changes

luisramos0 approved these changes Feb 3, 2019

View reviewed changes

kristinalim added the pr-staged-au staging.openfoodnetwork.org.au label Feb 4, 2019

kirstenalarsen removed the pr-staged-au staging.openfoodnetwork.org.au label Feb 8, 2019

kristinalim added 8 commits February 8, 2019 21:26

Add validator for datetime string

de9cff6

Example usage: validates :start_at, date_time_string: true

Add validator for integer arrays

f6e8f18

Example usage: validates :related_post_ids, integer_array: true

Add datetime and integer array validator matchers

519a7d2

Add basic form object for enterprise fee summary

a097640

Add data classes for Enterprise Fee Summary report

3763cb9

Specify sort order of enterprise fee totals data

faf7079

Add processing of enterprise fee summary from DB

e7ed625

Add test data for enterprise fee summary report

00e7073

kristinalim added 19 commits February 8, 2019 21:26

Move engine libraries except Engine and Version to services

c81be75

Address Rubocop violation

6378afd

Do not assign I18n translation to constants

eaa7800

The preferred language could change dynamically.

Do not include incomplete orders in report

4a73b1d

Clean up setup data for enterprise fee summary tests

53c458f

Add enterprise fee factory trait for per item rate

61ec43f

We should not presume what is not default for an enterprise fee object.

Use factory for tax category in entperprise fee summary tests

5dda3b6

Use enterprise fee factory trait where applicable

a3aa594

Move order setup to new factory traits

ce030ae

Rename helper for setting up incomplete order

f36d13a

Move variant setup to new factory trait

d2e49b8

Add :per_item factory trait to shipping and payment methods

625069c

Rename files for factories in spec/factories/

c92d202

Make these follow the "#{SINGULAR_OBJECT}_factory.rb" convention.

Add scenario test for enterprise fee summary

ddf15f1

Add test for more complex scenario where there is a coordinator and distributor fee for an incoming exchange, and a producer and coordinator fee for an outgoing exchange.

Use receiver as "Fee Calc on Transfer Through" for outgoing exchanges

f59834d

Add feature flag for enterprise fee summary

1d3f4aa

Hide report link and resource from non-superadmins

461b572

Fix description for feature flag example groups

de30cf1

Fix layout violation in Spree::Ability decorator

b6be8c4

luisramos0 force-pushed the feature/enterprise_fee_summary branch from 3200492 to b6be8c4 Compare February 8, 2019 21:27

kristinalim mentioned this pull request Feb 12, 2019

3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

Merged

sauloperez merged commit a0c144f into master Feb 12, 2019

mkllnk deleted the feature/enterprise_fee_summary branch May 6, 2019 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master #3415

[Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master #3415

kristinalim commented Jan 31, 2019 •

edited

Loading

kristinalim commented Jan 31, 2019

kristinalim commented Jan 31, 2019

kirstenalarsen commented Feb 1, 2019

mkllnk left a comment

kristinalim commented Feb 1, 2019 •

edited

Loading

luisramos0 left a comment

kirstenalarsen commented Feb 8, 2019

kirstenalarsen commented Feb 8, 2019

sauloperez commented Feb 8, 2019

sauloperez commented Feb 8, 2019 •

edited

Loading

luisramos0 commented Feb 8, 2019

kristinalim commented Feb 8, 2019

kristinalim commented Feb 8, 2019

kirstenalarsen commented Feb 9, 2019

sauloperez commented Feb 11, 2019

luisramos0 commented Feb 12, 2019

kristinalim commented Feb 12, 2019

luisramos0 commented Feb 12, 2019

kristinalim commented Feb 12, 2019

[Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master #3415

[Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master #3415

Conversation

kristinalim commented Jan 31, 2019 • edited Loading

What? Why?

What should we test?

Release notes

Discourse thread

Dependencies

kristinalim commented Jan 31, 2019

kristinalim commented Jan 31, 2019

kirstenalarsen commented Feb 1, 2019

mkllnk left a comment

Choose a reason for hiding this comment

kristinalim commented Feb 1, 2019 • edited Loading

luisramos0 left a comment

Choose a reason for hiding this comment

kirstenalarsen commented Feb 8, 2019

kirstenalarsen commented Feb 8, 2019

sauloperez commented Feb 8, 2019

sauloperez commented Feb 8, 2019 • edited Loading

luisramos0 commented Feb 8, 2019

kristinalim commented Feb 8, 2019

kristinalim commented Feb 8, 2019

kirstenalarsen commented Feb 9, 2019

sauloperez commented Feb 11, 2019

luisramos0 commented Feb 12, 2019

kristinalim commented Feb 12, 2019

luisramos0 commented Feb 12, 2019

kristinalim commented Feb 12, 2019

kristinalim commented Jan 31, 2019 •

edited

Loading

kristinalim commented Feb 1, 2019 •

edited

Loading

sauloperez commented Feb 8, 2019 •

edited

Loading