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

import/export Taxonomy API functions #1

Closed
wants to merge 7 commits into from

Conversation

ChrisChV
Copy link
Member

@ChrisChV ChrisChV commented Jun 21, 2023

Description

Implements import_tags and export_tags functions

Supporting information

Temporal PR until openedx#57 will merged

Testing instructions

  • Ensure that the tests cover the expected behaviour

Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

This is looking better, thank you @ChrisChV ! Couple more changes requested, but once they're resolved, this should be good to go.

openedx_tagging/core/tagging/api.py Outdated Show resolved Hide resolved
f"Invalid JSON format: Missing '{key}' on a tag ({tag})"
)
)
resync_tags()
Copy link
Member

Choose a reason for hiding this comment

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

I added an optional parameter to resync_tags which will speed up this call:

Suggested change
resync_tags()
resync_tags(ObjectTag.objects.filter(taxonomy=taxonomy))

You'll need to merge the latest changes from openedx#57 over here to use it though.

openedx_tagging/core/tagging/api.py Outdated Show resolved Hide resolved
Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

@ChrisChV This branch looks great, thank you!

I've squashed and rebased openedx#57 and am hoping that will be merged tomorrow.. so I think you can wait until that's merged and resurrect openedx#58, moving these changes over there?

Sorry for all the changes on my branch.. it's hard to work in parallel like this, but was the best use of time I think.

👍

  • I tested this by running the tests on my local machine and ensuring they cover all the code.
  • I read through the code carefully and checked that it covers the required functionality.
  • I checked for accessibility issues N/A
  • Includes documentation
  • Commit structure follows OEP-0051

pomegranited and others added 7 commits June 27, 2023 00:10
Also:
* Adds compile-requirements make target and updates (without upgrading) the requirements,
  which pulled in some other requirements, since it's been a while since
  they were compiled.
* Adds ddt as a test dependency.
* Replaces the placeholder TagContent class, and squashes migrations down to a single file
* use the openedx_learning.lib multi-collation char and text fields
Also:
* Adds rules requirement and app settings to enable it
* Adds mock to test requirements, so we can test system taxonomy rules
* ADR: Clarifies that rules will be enforced in the views, not the model or APIs
Tests are failing in openedx on sqlite.
@ChrisChV ChrisChV force-pushed the chris/taxonomy-import-export branch from a043b41 to e5b1698 Compare June 27, 2023 21:50
@ChrisChV ChrisChV closed this Aug 2, 2023
@ChrisChV
Copy link
Member Author

ChrisChV commented Aug 2, 2023

Closed in favor of openedx#64

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