Allow user to show/hide totals with a checkbox toggle

[lin-d-hop]

Description

To increase the utility of this report we wish to enable the user to show and hide the Totals rows (as displayed in the CSV mock)

The user will have an additional checkbox filter:

• Display Total Rows (Checkbox) Include total rows in display/CSV

When the box is checked, total row will be included.
When the box is not check the CSV download and the UI report will not have totals rows

This needs to be applied to both the By Producer and By Order pivots of this report.

Issue #6846 will be very similar to this.

Acceptance Criteria & Tests

1. The Display Total Rows checkbox is included
2. When it is ticked the totals rows are display in CSV download and on UI
3. When it is not ticked the totals rows are not displayed in CSV download and on UI
4. Fields are translatable

[abdellani]

Hi @lin-d-hop

Does "Display Total Rows" refer to "Summary Rows"?

In the report code, I see that if the selected format is CSV or JSON, the summary rows flag will be unchecked.

openfoodnetwork/lib/reporting/report_renderer.rb

Lines 24 to 28 in ee70645

	def display_summary_row?
	@report.params[:display_summary_row].present? && !raw_render?
	end

openfoodnetwork/lib/reporting/report_renderer.rb

Lines 13 to 15 in ee70645

	def raw_render?
	@report.params[:report_format].in?(['json', 'csv'])
	end

I noticed the same thing in the UI

Do you want to change that behavior?

[lin-d-hop]

Hi @abdellani
Correct, the 'total rows' are now called the 'summary rows'.
Let's stick with behaviour consistent to the other reports and disable the 'summary rows' in the CSV and JSON formats.

Out of interest, do you think it would be difficult to change this behaviour so that you can choose to show summary rows on CSV and JSON formats? This fell out of scope in the last reports changes and we had the impression it was a tricky to include the totals rows, particularly for the JSON. Don't spend more than an hour thinking about this question :)

[abdellani]

@lin-d-hop

To answer your question, I updated the report to authorize summary rows on CSV.
I tested the two reports: Sales Tax Totals By Order && Order Cycle Supplier Totals, and I didn't notice any problems.

Sales Tax Totals By Order

Order Cycle Supplier Totals

[lin-d-hop]

Cool, any idea about the JSON? Given there's still 30mins left in that hour ;)

[abdellani]

@lin-d-hop

This is the result: https://gist.github.com/abdellani/4ae748bd891ae99e81fa2f509c65e8cc

I think we have a small problem, the name of the attributes in the summary sub-document.

        {
            "producer": "",
            "product": "",
            "variant": "TOTAL",
            "quantity": 6,
            "total_units": "6.0",
            "curr_cost_per_unit": "",
            "total_cost": "420.0"
        }

I notice another problem.
I used the following query to request the documents
http://localhost:3000/api/v0/reports/orders_and_fulfillment?token=9ddde4af9a4f457d86e1c6fbd61d9880277c3ceadf67d69f&report_subtype=order_cycle_supplier_totals&q[order_cycle_id][]="5"&display_summary_row=true

I remove q[order_cycle_id][]="5" to fetch all the document
I get the following error

{
    "error": "Please supply Ransack search params in the request"
}

If I replace it with q[random][], it works.
q[][] doesn't solve the problem.

[lin-d-hop]

Thanks @abdellani! From a dev perspective, how hard would it be for us to create a search param like 'include totals' which if true returns a new sub document appended to the end with different attributes? This needs some thought and design, but in theory would this be difficult? Though our hour is up :)

It's a known thing that you need to supply some search params for the API RQ, and I think a good buffer to try stop an inexperienced API dev from crashing our servers with a request for too much data :)

[abdellani]

@lin-d-hop

We already have that search param: display_summary_row=true

One possible way to do it is by updating the extract_rows. We can check the targeted format before building the rows.

openfoodnetwork/lib/reporting/report_rows_builder.rb

Lines 47 to 65 in ee70645

	def extract_rows(data, result)
	data.each do |group_or_row|
	if group_or_row[:is_group].present?
	# Header Row
	if group_or_row[:header].present? && report.display_header_row?
	result << OpenStruct.new(header: group_or_row[:header])
	end
	# Normal Row
	extract_rows(group_or_row[:data], result)
	# Summary Row
	if group_or_row[:summary_row].present? && report.display_summary_row?
	result << group_or_row[:summary_row]
	end
	else
	result << group_or_row.row
	end
	end
	result
	end

[dacook]

Hi @abdellani , sorry I didn't realise you had already started on this.

It sounds like we don't have any reason for raw_render? to disallow summary rows for any format.
Based on your comments, it sounds like we can already allow summary rows on CSV, and with my PR, we can also enable for JSON (although it's certainly not pretty!)

So @lin-d-hop after my PR, should we proceed with allowing summary rows and headers on all formats?
We can remove this distinction between "Formatted data" and "raw data":

For backwards-compatibility, we should probably continue to make summary rows off by default for CSV.

[abdellani]

@dacook
I don't know if this is helpful:

• After this comment, I remember that I removed the summary row from a CSV file used on the specs.
• the UI disabled automatically the summary row checkbox when CSV format is selected.

[RachL]

It sounds like we don't have any reason for raw_render? to disallow summary rows for any format.

Just FYI, the use case to keep .csv without formatting was to easily allow csv to be imported elsewhere. Summary and total rows makes .csv less readable as these rows are not properly defined (you can't link a header row to an order for example, unless you are a human and you see the two are linked).

If we are making changes towards having both .csv and .xlsx version 100% alike I would suggest we drop maintaining .csv. It's easy to save in .csv from a .xslx version.

What do you think?

[dacook]

This is just my opinion, but I think:

• There's no technical reason we need to block summary rows on CSV
• There is still a reason to deselect them by default: CSV is usually chosen for importing to another system which would require raw data
• There is still a reason to allow download of CSV instead of XLSX: it requires an extra step for the user to convert the file (I imagine waiting 5 mins for MS Excel to boot up..). I think the maintenance cost of CSV is quite low for us (it's just a different option on the same gem).

Unless there's high risk of mistakes, I don't think we should place arbitrary limits on what people can do.
So I suggest we simply allow summary rows on CSV, but have it de-selected by default (like it does already). From what I can see, that's what's needed for this issue.

I also think header rows should be treated in the same way. I notice that we currently have an inconsistency: you can choose them for CSV but they never appear. It should be deselected by default but allowed if selected.

[RachL]

It should be deselected by default but allowed if selected.

Yes it's covered by #9546

I think the maintenance cost of CSV is quite low for us

Perhaps on the tech side, but we shouldn't forget testing. @filipefurtad0 what do you think?

[filipefurtad0]

I think the maintenance cost of CSV is quite low for us

Perhaps on the tech side, but we shouldn't forget testing

Good points. It seems to me that CSV is easy to test/maintain - we do it already in our suite (like for the product import feature).

I would not know how to test XLSX files for content and we have issues testing PDFs as well, so I'd say these are more challenging to maintain.

[RachL]

FYI product import allows xslx 👍

[filipefurtad0]

FYI product import allows xslx +1

Yes, but I don't think we test it 🙈

[lin-d-hop]

I do agree that if it isn't onerous to include the summary option in the CSV version we should do it. it will remove confusion. Given that we now allow this on API version, CSV is the only one left.

The original issue here is soooo outdated.... from before the new reports framework. I can create a new issue for this actual task and close this one.