Skip to content
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

Disable header row bulk_coop report #9563

Merged

Conversation

aintluks
Copy link
Contributor

@aintluks aintluks commented Aug 13, 2022

What? Why?

CSV reports: Grey out the option to add summary lines when csv is selected
Closes #9291

What should we test?

  • Visit page /admin/reports/bulk_coop
  • Select the Generate Report with 'csv'
  • You should not be able to select 'summary row' in display field
    Captura de tela de 2022-08-16 16-33-32

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@binarygit
Copy link
Contributor

Hello @aintluks Welcome to OFN 🎉

I'm not sure why you disabled the header row after CSV is selected because the goal is to disable the summary row. It's mentioned here: #9291 (comment) I think, maybe you got confused 🙂

@aintluks
Copy link
Contributor Author

Hello @aintluks Welcome to OFN tada

I'm not sure why you disabled the header row after CSV is selected because the goal is to disable the summary row. It's mentioned here: #9291 (comment) I think, maybe you got confused slightly_smiling_face

Yep, I got confused there.. my bad :p

@aintluks aintluks force-pushed the bulk-coop-report-csv branch from 0aff406 to 2a9db0e Compare August 16, 2022 19:32
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment on lines +4 to +7
static targets = ["reportType", "checkbox", "label"]

handleSelectChange() {
this.reportTypeTarget.value == "csv" ? this.disableField() : this.enableField()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making this more generic? If you supplied the value "csv" to the controller then it could work with any input field and disable any other form elements.

A generic controller would need to iterate through all targets and apply an action depending on their type. Checkboxes get checked, labels get a class, all inputs and buttons get disabled. This can be extended if needed.

You don't need to do this here now. It's just an idea to build more generic functionality which can be re-used instead of creating new controllers all the time.

@filipefurtad0 filipefurtad0 self-assigned this Aug 17, 2022
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 17, 2022
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Aug 17, 2022

Hey @aintluks,

Thanks for picking this one up 👍

After staging the PR it is possible to confirm that the option to select the summary row is opted out:

image

This indeed generates CSV files without the summary row:

image

So, this PR changes the behavior for the checkbox for the summary row, for all the reports which display this option:

  • Bulk Co-Op and sub-reports
  • Orders & Fulfillment and sub-reports
  • Packing and sub-reports

The behavior for the header row is left untouched.

Looks good to me, merging! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV reports: Grey out the option to add summary lines when csv is selected
4 participants