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

[Temporary] feat: Taxonomy export menu #4

Conversation

ChrisChV
Copy link
Member

@ChrisChV ChrisChV commented Oct 19, 2023

Changes added:

  • ExportModal.
  • Export menu added. Note: Is hidden for system-defined, because the export API doesn't allow export system-defined.
  • Functions to make a call to the export API.
  • Flow to export a taxonomy.
  • System-defined tooltip.

Screenshots

image
image

Testing instructions

  • Verify the tooltip of the badge of a System-defined taxonomy.
  • Create a new Taxonomy on the CMS python shell or use the taxonomy-sample-data.
  • Go to the Taxonomy List page, you should be able to see the new taxonomy.
  • Click on the tree dots on the new taxonomy card and on Export.
  • Check the new Export modal.
  • Click on Export on the Export modal with CSV format.
  • Verify the downloaded file.
  • Click on Export on the Export modal with JSON format.
  • Verify the downloaded file.

Other info

KristinAoki and others added 6 commits October 16, 2023 14:58
Internal issue: https://2u-internal.atlassian.net/browse/TNL-11085

File upload gallery cards had distorted thumbnails. The goal is to fix this.
I centered the thumbnails instead of stretching them. I had to change the card layout a bit to do that, so in order to make everything look fine, I worked a bit on margins and paddings and font-size in order to bring this close to the figma mockups. It's not perfect because when you resize the browser window, the grid does some resizing that doesn't look as good as in figma, but I think it should be good enough for now.
…v1.175.0 (openedx#631)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Changed "Files & Uploads" page title to just "Files"
@ChrisChV ChrisChV changed the title [Temporal] feat: Taxonomy export menu [Temporary] feat: Taxonomy export menu Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (chris/FAL-3522-taxonomy-list-page@7132136). Click here to learn what that means.

❗ Current head 60b4359 differs from pull request most recent head 09aa8b6. Consider uploading reports for the commit 09aa8b6 to get more accurate results

Additional details and impacted files
@@                         Coverage Diff                          @@
##             chris/FAL-3522-taxonomy-list-page       #4   +/-   ##
====================================================================
  Coverage                                     ?   86.86%           
====================================================================
  Files                                        ?      384           
  Lines                                        ?     5779           
  Branches                                     ?     1270           
====================================================================
  Hits                                         ?     5020           
  Misses                                       ?      735           
  Partials                                     ?       24           

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

fileExtension = 'csv';
data = response.data;
}
downloadDataAsFile(data, contentType, `${name}.${fileExtension}`);
Copy link
Member

Choose a reason for hiding this comment

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

Calling the endpoint with the parameter download=1 you could get the filename and extension from the backend using the Content-Disposition and simplify a bit here.

Another alternative is to leave the front end as it is and remove the download option to simplify the backend.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rpenido We need to call the endpoint using Axios because the call needs to be authenticated. I have tried using download=1, but doesn't works, here is the explanation. I've added that as a comment.

Another alternative is to leave the front end as it is and remove the download option to simplify the backend.

I think yes, we need to remove that because it will no longer be necessary, but it would be done in a follow-up task, I'm already running out of time here CC @bradenmacdonald

Copy link
Member

@bradenmacdonald bradenmacdonald Oct 20, 2023

Choose a reason for hiding this comment

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

Doesn't the user have cookie authentication for the CMS domain? You don't want to call it as an API using Axios/fetch, you just want to send the user's browser to that URL. window.location.href = '/path/to/download/api?download=1 - like that. Or even better, use <a href="/path/to/api?download=1">Download</a>. And @rpenido is right, you shouldn't need to specify the filename.

Copy link
Member

Choose a reason for hiding this comment

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

I also thought that the authentication would be a problem, but @bradenmacdonald is right. Making a request directly to the backend using the authenticated browser section worked (in my setup).

Copy link
Member Author

Choose a reason for hiding this comment

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

@rpenido How did you make the request? I have tried window.location.href way but doesn't work. CC @bradenmacdonald

vokoscreenNG-2023-10-20_12-22-40.mp4

Copy link
Member

Choose a reason for hiding this comment

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

@ChrisChV Weird. It works for me - I just went to http://studio.local.overhang.io:8001/api/content_tagging/v1/taxonomies/1/export/?output_format=json&download=1 (the tutor devstack equivalent URL) in my browser and it downloaded a file.

If you go to http://localhost:18010 are you logged in to Studio?

Copy link
Member

Choose a reason for hiding this comment

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

@ChrisChV I think you are using an old version of openedx-learning, and you're missing this commit: openedx/openedx-learning@d66d629

Copy link
Member Author

Choose a reason for hiding this comment

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

If you go to http://localhost:18010/ are you logged in to Studio?

Yes, that is it. Now it works, thanks! That simplifies things

Copy link
Member

Choose a reason for hiding this comment

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

Your code worked here @ChrisChV.

@bradenmacdonald
Copy link
Member

Is hidden for system-defined, because the export API doesn't allow export system-defined

Why does the API not allow exporting system-defined taxonomies? I discussed this with Axim and we decided that some power users may want to export system-level taxonomies, and there's no reason to prevent it.

Not in scope to fix now though.

@ChrisChV ChrisChV force-pushed the chris/FAL-3528-taxonomy-export-menu branch from 667bf1e to 09aa8b6 Compare October 20, 2023 18:45
@ChrisChV ChrisChV closed this Oct 20, 2023
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.

7 participants