-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
Report summaries in JSON (API) output #10466
Report summaries in JSON (API) output #10466
Conversation
172cce4
to
7ba8499
Compare
lib/reporting/report_renderer.rb
Outdated
@@ -10,7 +10,13 @@ def initialize(report) | |||
@report = report | |||
end | |||
|
|||
# Strip headers and footers for these formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does footers refer to the summary rows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if raw_render?
is only used to decide about the header and summary. Maybe we can use a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right it should say "summary rows" rather than "footers".
I agree it would be good to have a better name. I'm not sure what is better though. Maybe render_data_only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment, but opted to avoid renaming the method because I hope to remove it anyway.
Actually no, that issue is to allow summary rows in CSV output. But it seems we don't quite have consensus yet. This PR is to allow summary rows in JSON. I don't think it depends on a decision on the above, and it does unblock our integration requirements in AU so I suggest we continue. I'll create an issue for reference. |
@abdellani , I've attached an issue and addressed one of your comments. Would you be able to complete your review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good :)
Thank you @dacook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice quick solution.
@@ -124,7 +124,7 @@ def check_report | |||
end | |||
|
|||
it "get correct data" do | |||
allow(subject).to receive(:raw_render?).and_return(true) | |||
allow(subject).to receive(:unformatted_render?).and_return(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line should have been in a later commit. Too late now. 🤷
It looks like it was expected that the price and amount columns would be formatted, but this is not currently the case. So I cleaned this up to be less ambiguous. If any of these columns were to be configured for formatting, this line could be added back to test for unformatted output.
…ndependently This will allow us to change the options in the following commit.
You wouldn't want it by default, but it can be enabled with a parameter like display_summary_row=true
This can be enabled in the reports API with the hidden parameter `fields_to_show[]=report_row_type` to help with parsing summary row output.
754fa54
to
c27974c
Compare
Hey @dacook, I think there are only three report types, which include the header and summary row options. These are:
For these three cases, I've tested both the API v0 requests; for all others only UI tests. A summary is found below: API V0 tests
Bulk Co-op Supplier Report
Bulk Co-op Allocation
Order Cycle Supplier Totals
Order Cycle Supplier Totals by Distributor Order Cycle Distributor Totals by Supplier I could not make this one work 🔴 I keep getting the error Are you able to try locally @dacook , and reproduce?
Order Cycle Customer Totals
Pack By Customer
Pack By Supplier Pack By Product
Adding the feedback label, on the red test-case above (Order Cycle Distributor Totals by Supplier). |
Great thorough testing @filipefurtad0! I looked through that case and I discovered the problem in the url:
It should be distributor :) |
Yaayy, That's it!
Thanks @dacook. Merging. |
What? Why?
This change allows you to optionally enable summary rows on JSON report output, via the reports v0 API.
Example output:
http://localhost:3000/api/v0/reports/packing?authenticity_token=****&q%5Border_completed_at_gt%5D=2022-10-01+00%3A00&q%5Border_completed_at_lt%5D=2023-02-22+00%3A00&q%5Border_distributor_id_in%5D%5B%5D=&report_subtype=supplier&report_format=&display_summary_row=true&fields_to_show%5B%5D=hub&fields_to_show%5B%5D=supplier&fields_to_show%5B%5D=customer_code&fields_to_show%5B%5D=first_name&fields_to_show%5B%5D=last_name&fields_to_show%5B%5D=product&fields_to_show%5B%5D=variant&fields_to_show%5B%5D=quantity&fields_to_show%5B%5D=report_row_type
What should we test?
Test to ensure no change in Admin Reports:
Test to ensure no change in API v0 reports:
Test to enable summary rows
display_header_row=true
display_summary_row=true
Extra rows should be output, similar to example above
Test to enable summary row markers
Summary rows look just like normal rows, so an additional marker can be used to differentiate them:
fields_to_show[]=report_row_type
The summary rows should have a new column/field:
"report_row_type": "summary"
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates
Yep: https://ofn-user-guide.gitbook.io/ofn-api-handbook/ofn-api-v0-unsupported/reports