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

feat(reports): adding flag for removing the index on the report CSV #29335

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

Conversation

SkinnyPigeon
Copy link
Contributor

SUMMARY

Some of our account managers send their reports in CSV format. Currently, the index is shown in the CSV due to the POST_PROCESSED value being set in the call to the API. This is causing a few complaints from our clients due to the need for them to remove the column when working with the report data.

There is a current PR that has been ongoing for over a year and appears to have stalled. I have linked this below. Since this is unlikely to be completed, I am opening this in the hopes that we can resolve it sooner. As opposed to their approach, I reformat the url used to call the API based on the CSV_INDEX, leaving it as-is for its default behaviour.

Issue #22981
Current PR

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Superset Index

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added alert-reports Namespace | Anything related to the Alert & Reports feature data:csv Related to import/export of CSVs labels Jun 23, 2024
Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.75%. Comparing base (76d897e) to head (8e4dad8).
Report is 353 commits behind head on master.

Files Patch % Lines
superset/commands/report/execute.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #29335       +/-   ##
===========================================
+ Coverage   60.48%   83.75%   +23.26%     
===========================================
  Files        1931      519     -1412     
  Lines       76236    37637    -38599     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31524    -14590     
+ Misses      28017     6113    -21904     
+ Partials     2105        0     -2105     
Flag Coverage Δ
hive 48.90% <28.57%> (-0.27%) ⬇️
javascript ?
mysql 77.25% <35.71%> (?)
postgres 77.38% <35.71%> (?)
presto 53.50% <28.57%> (-0.31%) ⬇️
python 83.75% <92.85%> (+20.27%) ⬆️
sqlite 76.84% <35.71%> (?)
unit 59.21% <71.42%> (+1.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SkinnyPigeon
Copy link
Contributor Author

SkinnyPigeon commented Jun 24, 2024

I did wonder about making the remove_post_processing function more general use to allow other mutations to URLs based on flags across the repo. For instance, it could be something like:

def mutate_api_call(url: str, parameter: str) -> str:
    
    Args:
        url (str): The url to process.
        parameter (str): The parameter to remove
    Returns:
        str: The url with the parameter removed."""
    if "?" not in url:
        return url
    base_url, query_string = url.split("?", 1)
    params = query_string.split("&")
    filtered_params = [param for param in params if param != parameter]
    filtered_query_string = "&".join(filtered_params)
    filtered_url = f"{base_url}?{filtered_query_string}"
    return filtered_url

I would then move it into a more general utils location. It would then be called in the same place but changed to:

if not current_app.config["CSV_INDEX"]:
    url = mutate_api_call(url, "type=post_processed")

@john-bodley
Copy link
Member

@SkinnyPigeon do you think this would be better handled at the report level as opposed from a system wide configuration?

@rusackas rusackas requested a review from eschutho June 24, 2024 17:21
@SkinnyPigeon
Copy link
Contributor Author

Yes, you're probably correct 😄. I doubt I would have the capacity to do the changes across both the frontend and backend to achieve that. What are the recommendations for getting a piece of work like that done on this repo?

@michael-s-molina
Copy link
Member

@SkinnyPigeon this looks more like a configuration of your report than a global setting.

We should include an option here. @eschutho

Screenshot 2024-06-25 at 10 56 37

@SkinnyPigeon
Copy link
Contributor Author

Thanks all. I will try to give it a look but if there's anyone you know in the community that's more familiar with working with the modals in the frontend, feel free to pass their names on and I am more than happy to reach out to them and see if we can work together to get this done

@michael-s-molina
Copy link
Member

if there's anyone you know in the community that's more familiar with working with the modals in the frontend

I believe @fisjac @rtexelm @dpgaspar recently worked on these modals.

@rusackas
Copy link
Member

rusackas commented Jul 9, 2024

CC @eschutho @yousoph in case they have any interest in helping to carry this forward. It's so close! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alert-reports Namespace | Anything related to the Alert & Reports feature data:csv Related to import/export of CSVs size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants