openfoodnetwork icon indicating copy to clipboard operation
openfoodnetwork copied to clipboard

[Reports] Ability for the admin to save their rendering options per reports

Open RachL opened this issue 2 years ago • 6 comments

Description

- As a: enterprise user or super admin - On page: each sub pages of admin/reports - I want to be able to do: Be able to save the last 'rendering options' I have used

Rendering Options include:

  • Report Type
  • Display (Header/Summary rows)
  • Columns to Show (see here)
  • Report Generation type

The same rendering options exist on each report, however some of the options available change on a report by report basis.

Acceptance Criteria

  • For each report, each of the rendering options is saved such that when I click away from the report, or log out and log back in, the rendering options I used last time are shown.
  • If I set rendering options on one report, and set rendering options on a second report, then both are saved independently from each other.
  • Rendering options are saved per user (ie not per enterprise)
  • Functionality works on all reports, including super admin reports
  • Settings are saved on click. There is no save button. Saving is automatic.
  • Functionality is covered by automated tests (see comments, it was agreed that testing each rendering option rather than each report is acceptable)

Testing notes

  1. Log in as enterprise user
  2. Go to admin/reports
  3. Select a report and add rendering options
  4. Launch the display of the report
  5. Go to another page and come back : see the rendering you have selected are still in place.
  6. Saving does not work if you did not display the report

RachL avatar Apr 29 '22 12:04 RachL

@jibees will check if this is a papercut or not. Proposed solution : always save, no checkbox. Saving is done when clicking on go.

RachL avatar May 11 '22 09:05 RachL

I'd vote for a paper-cut in term of development. Creating all the specs for all the rendering options for each reports could be very time-consuming. Maybe we should assume to create only for the reports that had the greatest number of options?

jibees avatar May 12 '22 08:05 jibees

@jibees I can be mistaken but it looks to me like all reports have the same number of options. Unless you count the ability to hide 1 column as one? In that case, it depends on the number of columns one report has.

RachL avatar May 17 '22 09:05 RachL

You're right, it should consist to only create a spec for one report, and see if it's well saved. Not so time consuming finally.

jibees avatar May 17 '22 12:05 jibees

@jibees one more question came up when I was writing the acceptance criteria : there are some rendering options which are similar across reports like "summary row". When I save "summary row" for one report does it save it on ALL reports? For now, I think from the product side we would prefer to have it per reports.

RachL avatar May 17 '22 15:05 RachL

I'd say per report too...

jibees avatar May 18 '22 06:05 jibees

Hi

As the rendering options will persist after the logout, I was thinking about adding a new model to store the rendering options. What do you think?

ReportRenderingOptions
  report_type: string
  report_subtype: string
  user: reference
  options: text #serialized hash

abdellani avatar Nov 15 '22 08:11 abdellani

Yes, I think this should do the job. We want a tuple with Report-Subreport/User/Rendering options. I think it's good then.

jibees avatar Nov 15 '22 09:11 jibees

I noticed that when I access the report page for the first time after clicking a link from the reports list, the backend generates the report even if nothing is rendered. This will run unnecessary queries to the database, no?

This is question is maybe not related directly to the feature we want to develop here.

abdellani avatar Nov 16 '22 10:11 abdellani

the backend generates the report even if nothing is rendered. This will run unnecessary queries to the database, no?

Yes, for sure. This is something we need to facing on. But not related to this issue here. If you have any solution, ... feel free to open a new pull request!

jibees avatar Nov 16 '22 10:11 jibees

I noticed that when I access the report page for the first time after clicking a link from the reports list, the backend generates the report even if nothing is rendered.

Hi @abdellani, just wondering if you confirmed this bug or not? If so, could you please raise it?

dacook avatar Nov 23 '22 05:11 dacook

@dacook As far as I understood it, it's unfortunately how reports were created originally. I'm not sure we can treat it as a bug? Unless it's super easy to fix...

RachL avatar Nov 23 '22 14:11 RachL

Hi @dacook

The data fetched from the database is not used. I wouldn't consider it as a problem if the queries don't impact the production server performance.

abdellani avatar Nov 23 '22 14:11 abdellani

From what I can see, the report class is automatically instantiated, but it's not immediately clear to me how it works. It sounds like you're saying it runs the report (query the database) based on the default form parameters. That doesn't sound good, but if it's not a problem, then I won't look into it further :)

dacook avatar Nov 23 '22 23:11 dacook

@dacook

it runs the report (query the database) based on the default form parameters.

Yes, you're right.

That doesn't sound good, but if it's not a problem, then I won't look into it further

I can't say if it's a problem or not.

abdellani avatar Nov 24 '22 06:11 abdellani

That doesn't sound good, but if it's not a problem, then I won't look into it further

It's a problem but it's out of the scope of this PR. If it improves performance we can take a look into it in the next performance issues.

RachL avatar Nov 24 '22 08:11 RachL