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

fix(Report) - Export csv/pdf with date as exposed filter #4858

Open
wants to merge 1 commit into
base: hotfix
Choose a base branch
from

Conversation

re8260
Copy link
Contributor

@re8260 re8260 commented Jan 7, 2018

Description

image

Motivation and Context

Bug fix, empty export CSV/PDF of report with exposed date filter

How To Test This

  1. Create new report
  2. Add date condition
  3. check checkbox to let user change filter from report
    image
  4. got to report, and export it as CSV or PDF

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

Copy link
Contributor

@PedroErnst PedroErnst left a comment

Choose a reason for hiding this comment

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

This fix works for the csv export.
However it doesn't work for the pdf (you would need to replicate the same changes for the function bound to 'download_pdf_button_old') just above these changes. You might want to extract all of it into a function to avoid code duplication.

Two tiny nitpicks:

  • Indentation should be with 2 spaces instead of tabs as described in the code style
  • Mentioning the issue in a comment is generally not considered best practice. I'd say in this case it's all pretty self explanatory.

Thanks for submitting this commit!

@gymad gymad added the PR:Wrong Branch Pull requests that point towards a restricted branch such as master label Aug 3, 2018
@re8260 re8260 changed the base branch from master to hotfix August 8, 2018 15:23
@jack7anderson7 jack7anderson7 added the PR:Community Contribution These are contribution made by the community label Dec 3, 2018
@Dillon-Brown Dillon-Brown removed the PR:Wrong Branch Pull requests that point towards a restricted branch such as master label Apr 5, 2019
@Mac-Rae
Copy link
Contributor

Mac-Rae commented Aug 5, 2019

Please rebase this pull request onto hotfix-7.10.x as the hotfix branch is now specifically for 7.11.x issues.

Please take a look at our contribution guidelines for more information.

@Mac-Rae Mac-Rae added the PR:Wrong Branch Pull requests that point towards a restricted branch such as master label Aug 5, 2019
@SuiteBot
Copy link

SuiteBot commented Aug 27, 2020

CLA assistant check
All committers have signed the CLA.

@jack7anderson7 jack7anderson7 added the Outdated: Needs Review PR needs reviewed as it might be outdaded label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Outdated: Needs Review PR needs reviewed as it might be outdaded PR:Community Contribution These are contribution made by the community PR:Wrong Branch Pull requests that point towards a restricted branch such as master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants