feat: Taxonomy export menu [FC-0036] #645

ChrisChV · 2023-10-19T18:46:16Z

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

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

Internal ticket: FAL-3528.
Closes [Tagging] Per-taxonomy menu on Taxonomy List Page modular-learning#105

openedx-webhooks · 2023-10-19T18:46:22Z

Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2023-10-20T18:55:03Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (77a355e) 88.02% compared to head (16233fb) 88.40%.
Report is 14 commits behind head on master.

Files	Patch %	Lines
src/taxonomy/taxonomy-card/TaxonomyCardMenu.jsx	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
+ Coverage   88.02%   88.40%   +0.38%     
==========================================
  Files         417      424       +7     
  Lines        6598     6573      -25     
  Branches     1428     1411      -17     
==========================================
+ Hits         5808     5811       +3     
+ Misses        765      737      -28     
  Partials       25       25

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

src/taxonomy/api/hooks/api.test.js

src/taxonomy/api/types.mjs

rpenido

Hi @ChrisChV! It looks good to me.

I read through the code
I followed the test instructions and ran the code locally

About the system-defined taxonomy export that @bradenmacdonald mentioned in the other PR:
We have a restriction in the python API blocking the import/export of system-defined taxonomies. I think the main reason was to avoid importing a system-defined taxonomy (no one can change it), but currently, the same check is applied to both (import/export):

https://github.com/openedx/openedx-learning/blob/v0.2.5/openedx_tagging/core/tagging/import_export/api.py#L145-L151

I think it will not be a problem to decouple the checks.

@bradenmacdonald Do you think we should handle it in this task or another later?

pomegranited · 2023-10-25T02:38:07Z

@ChrisChV CC @rpenido This repo has good documentation in the README describing its various features and how to enable them. This isn't the first Tagging feature for this repo (#622 was first), but this is the next one likely to merge, so could you update the README to add details for these features (including the platform flags we need to set), and we'll do the same for subsequent Tagging PRs?

bradenmacdonald · 2023-10-26T18:22:41Z

@bradenmacdonald Do you think we should handle it in this task or another later?

Let's make the change to allow export (not import) of system-defined taxonomies on the backend now. It should be a simple change, and you don't need to include a version bump or anything. Later we can make the corresponding UI change in a separate task.

rpenido · 2023-10-26T21:35:28Z

@bradenmacdonald Do you think we should handle it in this task or another later?

Let's make the change to allow export (not import) of system-defined taxonomies on the backend now. It should be a simple change, and you don't need to include a version bump or anything. Later we can make the corresponding UI change in a separate task.

Done: openedx/openedx-learning#107
I assumed that the same principle was valid for open taxonomies (allow_free_text=True)

ChrisChV · 2023-10-30T16:55:59Z

@ChrisChV CC @rpenido This repo has good documentation in the README describing its various features and how to enable them. This isn't the first Tagging feature for this repo (#622 was first), but this is the next one likely to merge, so could you update the README to add details for these features (including the openedx/edx-platform#33439), and we'll do the same for subsequent Tagging PRs?

Thanks @pomegranited, I added it on 5af8d66

xitij2000 · 2023-10-31T10:07:14Z

src/taxonomy/api/hooks/selectors.js

+
+export const callExportTaxonomy = (pk, format) => (
+  exportTaxonomy(pk, format)
+);


I don't think selectors is the correct place for this, although I think this might be a preexisting problem with this module.

I think it's important to keep this ADR in mind: https://github.com/openedx/frontend-template-application/blob/master/docs/decisions/0002-feature-based-application-organization.rst

None of the functions here are hooks or selectors in the commonly understood context of those terms.

@xitij2000 I updated the UI components to match with the ADR here: b09b86a

About the data layer, we have try to use useQuery to get data. On df8432d I tried to match with the structure on the ADR, @xitij2000 could you check it?

CC @bradenmacdonald

Given that this is using useQuery instead of redux I'm not sure how things would map. However, I think it's best to avoid confusion with exist terms like hooks, thunks, selectors, if you are not using those since someone else working on this might have difficulty navigating this codebase in that case.

Thanks! I understand. I deleted thunks.js and I move hooks.jsx to api/apiHooks.jsx (to avoid use hooks or any other exist term). Also I added a docstring in api/apiHooks.jsx to explain all of this.

xitij2000 · 2023-10-31T10:09:24Z

src/taxonomy/taxonomy-card/TaxonomyCard.jsx

+  const orgsCountEnabled = () => orgsCount !== undefined && orgsCount !== 0;
+
+  const onClickMenuItem = (menuName) => {
+    switch (menuName) {


If there is only one option, wouldn't this be clearer as a simple if condition?

In future tasks more menu items will be added. I have added a comment for it

Sure, that makes sense. Would it be cleaner in this case to have a dict based approach?
i.e. something like this:

const menuItemActions = { 'export': setIsExportModalOpen, 'import': setIsImportModalOpen, ... } menuItemActions[menuName]?.(true)

This might end up being less clear depending on the usage though since I don't know the whole context.

Thanks for the suggestion. I like it. I added it on this commit: 080b03e

src/taxonomy/taxonomy-card/TaxonomyCard.jsx

src/taxonomy/modals/ExportModal.test.jsx

bradenmacdonald · 2023-11-03T17:11:42Z

@ChrisChV Is this ready for another look from @xitij2000 ? Or are you still working on refactoring the tests?

ChrisChV · 2023-11-03T19:55:30Z

@bradenmacdonald Yes, I will check this way. @xitij2000 do you have any comments about the changes here? or recommendations? So I can do changes in one batch :)

ChrisChV · 2023-11-06T16:20:49Z

@xitij2000 I updated the tests. It's full ready for another review

pomegranited · 2023-11-07T23:51:38Z

src/taxonomy/data/thunks.js

+/* istanbul ignore next */
+const exportTaxonomy = (pk, format) => (
+  getTaxonomyExportFile(pk, format)
+);


@ChrisChV IIRR, thunk.js is for redux operations (ref). And since exportTaxonomy is just wrapping an API function, can't we just call getTaxonomyExportFile directly where it's needed?

Is that right @xitij2000 ?

Yes, thunks are where you can connect the API to the redux store. Since this is using a different approach with useQuery, I think there might not be any thunks here.

Thanks! Ok, I agree. I deleted thunks.js 080b03e

xitij2000

I'm currently not able to test this in running since my devstack is set up for something else I don't want to delay this.

Has it been manually tested?

xitij2000 · 2023-11-02T09:21:14Z

src/taxonomy/taxonomy-card/TaxonomyCard.jsx

+  const orgsCountEnabled = () => orgsCount !== undefined && orgsCount !== 0;
+
+  const onClickMenuItem = (menuName) => {
+    switch (menuName) {


Sure, that makes sense. Would it be cleaner in this case to have a dict based approach?
i.e. something like this:

const menuItemActions = { 'export': setIsExportModalOpen, 'import': setIsImportModalOpen, ... } menuItemActions[menuName]?.(true)

This might end up being less clear depending on the usage though since I don't know the whole context.

xitij2000 · 2023-11-08T07:08:52Z

src/taxonomy/data/thunks.js

+/* istanbul ignore next */
+const exportTaxonomy = (pk, format) => (
+  getTaxonomyExportFile(pk, format)
+);


Yes, thunks are where you can connect the API to the redux store. Since this is using a different approach with useQuery, I think there might not be any thunks here.

xitij2000 · 2023-11-08T07:13:49Z

src/taxonomy/api/hooks/selectors.js

+
+export const callExportTaxonomy = (pk, format) => (
+  exportTaxonomy(pk, format)
+);


Given that this is using useQuery instead of redux I'm not sure how things would map. However, I think it's best to avoid confusion with exist terms like hooks, thunks, selectors, if you are not using those since someone else working on this might have difficulty navigating this codebase in that case.

rpenido · 2023-11-08T13:38:46Z

I'm currently not able to test this in running since my devstack is set up for something else I don't want to delay this.

Has it been manually tested?

Hi @xitij2000 ! Yes!
I just tested it.

ChrisChV · 2023-11-08T16:00:43Z

@xitij2000 I addressed your comments. It's ready for another review

pomegranited · 2023-11-09T01:26:23Z

README.rst

+
+* ``edx-platform`` Waffle flags:
+
+  * ``contentstore.new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.


nit:

Suggested change

* ``contentstore.new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.

* ``new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.

This is correct, but all the other feature flags use contentstore.new_studio_mfe... so that was a small mistake on our part. Doesn't really matter though.

ChrisChV · 2023-11-13T16:47:07Z

Hi @xitij2000, kindly reminder to do another round of review here. We need to unblock and merge this. I suggest that if there is another recommendation on the architecture and structure of the taxonomy module, we could chek it and reach a consensus in another ticket/PR 😄

xitij2000

Looks good! Just one small nit.

src/taxonomy/taxonomy-card/index.jsx

ChrisChV · 2023-11-14T17:09:19Z

@bradenmacdonald @xitij2000 It's ready to merge. I don't have permission to do it

openedx-webhooks · 2023-11-14T18:08:41Z

@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

* feat: System-defined tooltip added * feat: Taxonomy card menu added. Export menu item added * feat: Modal for export taxonomy * feat: Connect with export API * test: Tests for API and selectors * feat: Use windows.location.href to call the export endpoint * test: ExportModal.test added * style: Delete unnecessary code * docs: README updated with taxonomy feature * style: TaxonomyCard updated to a better code style * style: injectIntl replaced by useIntl on taxonomy pages and components * refactor: Move and rename taxonomy UI components to match 0002 ADR * refactor: Move api to data to match with 0002 ADR * test: Refactor ExportModal tests * chore: Fix validations * chore: Lint * refactor: Moving hooks to apiHooks * style: Nit on return null --------- Co-authored-by: Rômulo Penido <[email protected]> Co-authored-by: Christofer Chavez <[email protected]>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 19, 2023

ChrisChV marked this pull request as draft October 19, 2023 18:46

ChrisChV added 6 commits October 20, 2023 18:42

feat: System-defined tooltip added

4963124

feat: Taxonomy card menu added. Export menu item added

4077397

feat: Modal for export taxonomy

e87c545

feat: Connect with export API

4922420

test: Tests for API and selectors

5e12a8f

feat: Use windows.location.href to call the export endpoint

09aa8b6

ChrisChV force-pushed the chris/FAL-3528-taxonomy-export-menu branch from 667bf1e to 09aa8b6 Compare October 20, 2023 18:45

ChrisChV marked this pull request as ready for review October 20, 2023 18:46

test: ExportModal.test added

1925fae

rpenido reviewed Oct 20, 2023

View reviewed changes

src/taxonomy/api/hooks/api.test.js Outdated Show resolved Hide resolved

rpenido reviewed Oct 20, 2023

View reviewed changes

src/taxonomy/api/hooks/api.test.js Outdated Show resolved Hide resolved

rpenido reviewed Oct 20, 2023

View reviewed changes

src/taxonomy/api/types.mjs Outdated Show resolved Hide resolved

rpenido approved these changes Oct 20, 2023

View reviewed changes

style: Delete unnecesary code

0be804c

rpenido mentioned this pull request Oct 24, 2023

[temp] feat: add detail taxonomy page open-craft/frontend-app-authoring#5

Closed

Merge branch 'master' into chris/FAL-3528-taxonomy-export-menu

da0156f

ChrisChV and others added 2 commits October 30, 2023 11:39

Merge branch 'master' into chris/FAL-3528-taxonomy-export-menu

0cc473c

docs: README updated with taxonomy feature

5af8d66

rpenido mentioned this pull request Oct 30, 2023

feat: bare bones taxonomy detail page [FC-0036] #655

Merged

xitij2000 reviewed Oct 31, 2023

View reviewed changes

ChrisChV added 2 commits October 31, 2023 17:19

style: TaxonomyCard updated to a better code style

a21fac2

style: injectIntl replaced by useIntl on taxonomy pages and components

61864d3

ChrisChV added 2 commits October 31, 2023 18:02

refactor: Move and rename taxonomy UI components to match 0002 ADR

b09b86a

refactor: Move api to data to match with 0002 ADR

df8432d

Christofer Chavez added 2 commits November 6, 2023 16:16

test: Refactor ExportModal tests

1abf034

chore: Merge conflicts solved

1d6f9a5

ChrisChV added 2 commits November 6, 2023 16:51

chore: Fix validations

5c4c87a

chore: Lint

7e514b6

pomegranited reviewed Nov 7, 2023

View reviewed changes

xitij2000 reviewed Nov 8, 2023

View reviewed changes

refactor: Moving hooks to apiHooks

080b03e

pomegranited reviewed Nov 9, 2023

View reviewed changes

bradenmacdonald mentioned this pull request Nov 13, 2023

feat: add download template button to taxonomy list [FC-0036] #674

Merged

xitij2000 approved these changes Nov 14, 2023

View reviewed changes

src/taxonomy/taxonomy-card/index.jsx Outdated Show resolved Hide resolved

xitij2000 mentioned this pull request Nov 14, 2023

[FC-0036] feat: Add ContentTagsDrawer widget #654

Merged

style: Nit on return null

16233fb

ormsbee merged commit 1ee80b6 into openedx:master Nov 14, 2023
6 checks passed

kenclary mentioned this pull request Jan 9, 2024

fix: configure webpack to fix paid powerpaste plugin #783

Merged

bradenmacdonald deleted the chris/FAL-3528-taxonomy-export-menu branch July 12, 2024 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Taxonomy export menu [FC-0036] #645

feat: Taxonomy export menu [FC-0036] #645

ChrisChV commented Oct 19, 2023 •

edited

Loading

openedx-webhooks commented Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 20, 2023 •

edited

Loading

rpenido left a comment •

edited

Loading

pomegranited commented Oct 25, 2023 •

edited

Loading

bradenmacdonald commented Oct 26, 2023

rpenido commented Oct 26, 2023

ChrisChV commented Oct 30, 2023

xitij2000 Oct 31, 2023

ChrisChV Oct 31, 2023

xitij2000 Nov 8, 2023

ChrisChV Nov 8, 2023

xitij2000 Oct 31, 2023

ChrisChV Oct 31, 2023

xitij2000 Nov 2, 2023

ChrisChV Nov 8, 2023

bradenmacdonald commented Nov 3, 2023

ChrisChV commented Nov 3, 2023

ChrisChV commented Nov 6, 2023

pomegranited Nov 7, 2023

xitij2000 Nov 8, 2023

ChrisChV Nov 8, 2023

xitij2000 left a comment

xitij2000 Nov 2, 2023

xitij2000 Nov 8, 2023

xitij2000 Nov 8, 2023

rpenido commented Nov 8, 2023

ChrisChV commented Nov 8, 2023

pomegranited Nov 9, 2023

bradenmacdonald Nov 9, 2023

ChrisChV commented Nov 13, 2023

xitij2000 left a comment

ChrisChV commented Nov 14, 2023

openedx-webhooks commented Nov 14, 2023


		* ``edx-platform`` Waffle flags:

		* ``contentstore.new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.

	* ``contentstore.new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.
	* ``new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.

feat: Taxonomy export menu [FC-0036] #645

feat: Taxonomy export menu [FC-0036] #645

Conversation

ChrisChV commented Oct 19, 2023 • edited Loading

openedx-webhooks commented Oct 19, 2023 • edited Loading

codecov bot commented Oct 20, 2023 • edited Loading

Codecov Report

rpenido left a comment • edited Loading

Choose a reason for hiding this comment

pomegranited commented Oct 25, 2023 • edited Loading

bradenmacdonald commented Oct 26, 2023

rpenido commented Oct 26, 2023

ChrisChV commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 3, 2023

ChrisChV commented Nov 3, 2023

ChrisChV commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Nov 8, 2023

ChrisChV commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV commented Nov 13, 2023

xitij2000 left a comment

Choose a reason for hiding this comment

ChrisChV commented Nov 14, 2023

openedx-webhooks commented Nov 14, 2023

ChrisChV commented Oct 19, 2023 •

edited

Loading

openedx-webhooks commented Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 20, 2023 •

edited

Loading

rpenido left a comment •

edited

Loading

pomegranited commented Oct 25, 2023 •

edited

Loading