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

[FC-0049] Handle tags when importing/exporting courses #34356

Merged

Conversation

ChrisChV
Copy link
Contributor

@ChrisChV ChrisChV commented Mar 12, 2024

Description

  • Export: Add tags.csv when exporting a course.
  • Import: Read tags.csv and creates object_tags depending on the scenario
  • Changes to use object_tag._export_id
  • Update export/import tags separator to ;

Supporting information

Testing instructions

  • Run make requirements on cms shell.
  • Run make migrate on cms shell.
  • Make sure you have Taxonomy/Tags data setup locally and verify that the command runs well: https://github.com/open-craft/taxonomy-sample-data/
  • Create a new/empty course.
  • Export empty course. Verify that tags.csv is not included in the export files.
  • Export a Sample Taxonomy Course. Verify that tags.csv is included in the export files. Open and verify the content of the file.

Scenario 1:

  • Import the new generated export files into the empty course. Verify that the tags are visible.

Scenario 2:

  • Open tags.csv and add tags that doesn't exist on a taxonomy (FlatTaxonomy in this example). Save.
  • Import with the edited tags.csv. Verify that the new tags are not visible.
  • Go to Taxonomy list page. Export FlatTaxonomy as csv and add the new tags. Import.
  • Verify that the new tag is visible on the course.

Scenario 3:

  • Open tags.csv and add tags that doesn't exist on a taxonomy that doesn't exist. You need to add a new column with a new export_id. Save.
  • Import with the edited tags.csv. Verify that the new tags are not visible.
  • Create a csv file to import a new taxonomy with the new tags
  • Go to Taxonomy list page. Import the new taxonomy with the same export_id added on tags.csv.
  • Open Manage organizations menu of the new taxonomy and add it to all organizations.
  • Verify that the new taxonomy and tags are visible on the course.

Scenario 4: openedx/modular-learning#204

Other information

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 12, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 12, 2024

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.

@ChrisChV ChrisChV marked this pull request as draft March 12, 2024 17:54
@ChrisChV ChrisChV force-pushed the chris/FAL-3624-import-export-courses branch 4 times, most recently from 3305ef2 to adb81c5 Compare March 14, 2024 22:38
@ChrisChV ChrisChV force-pushed the chris/FAL-3624-import-export-courses branch from adb81c5 to 82dab62 Compare March 19, 2024 18:50
@ChrisChV ChrisChV marked this pull request as ready for review March 26, 2024 19:16
@@ -170,10 +170,10 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict:
}
for obj_tag in all_tags:
# Add the taxonomy name:
if obj_tag.name not in result[Fields.tags_taxonomy]:
result[Fields.tags_taxonomy].append(obj_tag.name)
if obj_tag.taxonomy.name not in result[Fields.tags_taxonomy]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradenmacdonald On openedx/openedx-learning#172 _name has been changed to _export_id, but here I have kept the taxonomy name. That is ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's right. Here should be the taxonomy name only. Thanks :)

requirements/edx/kernel.in Outdated Show resolved Hide resolved
@ChrisChV
Copy link
Contributor Author

@rpenido @bradenmacdonald About the Scenario 4:

Tags will be imported and visible in Studio

In the tag drawer, by default taxonomies that are not part of the course organization are not displayed. Would this change on import? What I have in mind is to add a field to the ObjectTag: imported, if it is True and meets the conditions of this scenario, it would be displayed in the tag drawer. What do you think?

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Mar 26, 2024

@ChrisChV

Scenario 4: Tags in the OLX match a Taxonomy (by Export ID) but the current user does not have permission to view/use that taxonomy [because it's linked to a different org]:
Tags will be imported and visible in Studio. Course author will be able to delete them if they want. Course author will not be able to add additional tags from the same taxonomy though, because they don't have permission.

In the tag drawer, by default taxonomies that are not part of the course organization are not displayed. Would this change on import? What I have in mind is to add a field to the ObjectTag: imported, if it is True and meets the conditions of this scenario, it would be displayed in the tag drawer. What do you think?

No, you don't need to add an imported field. It shouldn't matter whether the tag was imported or not. Basically, any time that there are tags from a taxonomy that the user doesn't have permission to use, it should behave like this (whether imported or not): "Course author will be able to delete them if they want. Course author will not be able to add additional tags from the same taxonomy though, because they don't have permission."

CC @jmakowski1123 to confirm.

@ChrisChV
Copy link
Contributor Author

"Course author will be able to delete them if they want. Course author will not be able to add additional tags from the same taxonomy though, because they don't have permission."

Taxonomies/tags that course author don't have permission to view are currently not displayed. Can't delete. I would have to check if the taxonomy has tags and show it

@rpenido
Copy link
Contributor

rpenido commented Mar 27, 2024

Export empty course. Verify that tags.csv is not included in the export files.

Hi @ChrisChV ! While testing, the "empty" course has a language tag (auto-tagged). I removed the tag and exported the course again. I got a tags.csv with the following content:

"Name","Type","ID","-1-languages"
"Empty Course","course","course-v1:org1+empty1+001",""

I don't think this is an issue, but it differs from your test instructions (actually, I think this is good because the user can add new tags even if the course has none applied).

Besides that, everything works flawlessly. Good work, @ChrisChV! When you have Scenario 4, let me know so I can review it again.

@bradenmacdonald
Copy link
Contributor

Taxonomies/tags that course author don't have permission to view are currently not displayed. Can't delete. I would have to check if the taxonomy has tags and show it

If you want, we can just open a ticket for this on https://github.com/openedx/modular-learning/issues/ and tackle it later.

@ChrisChV
Copy link
Contributor Author

If you want, we can just open a ticket for this on https://github.com/openedx/modular-learning/issues/ and tackle it later.

@bradenmacdonald @rpenido I created open-craft#649 and openedx/frontend-app-authoring#926 as a proof of concept of the Scenario 4. I am missing the tests and testing instructions. I will continue it if you agree with the solution 😀

When you add a new tags in a course, the previous tags are not exported
How repodruce:
- Import a course
- Add new tags to the course or blocks
- Export a course
@bradenmacdonald
Copy link
Contributor

@ChrisChV Is there some way we could do it without changing the taxonomy permissions or retrieving "all taxonomies"? Perhaps just list all the objecttags and if any don't match the "allowed" taxonomies, we can display the rest under a heading like "Other Tags"

@ChrisChV
Copy link
Contributor Author

ChrisChV commented Apr 2, 2024

@bradenmacdonald I have realized that scenario 4 needs more discussion. I created openedx/modular-learning#204 and I continued the discussion there. This PR works and can be merged without Scenario 4 CC @rpenido

@bradenmacdonald
Copy link
Contributor

@rpenido Can you please review this? Then I can do a CC review + merge. We can handle "scenario 4" separately.

Copy link
Contributor

@rpenido rpenido left a comment

Choose a reason for hiding this comment

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

LGTM @ChrisChV ! 👍

  • I tested this using the Testing Instructions
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

This is ready for CC review @bradenmacdonald

tags_count = get_object_tag_counts(block_id_pattern)
course_tags_count = get_object_tag_counts(courselike_key_str)

if tags_count or course_tags_count:
Copy link
Contributor

Choose a reason for hiding this comment

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

I found the cause of having an empty tags.csv in my tests. At the auto-tagging handler, we tag the course (course-v1:...) and the "course block". The latter is not shown anywhere but probably appears in this count.

Could we create a small task to fix this @bradenmacdonald?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpenido Sure, thanks!

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Just a few minor things and then I'll approve and merge.

@@ -61,7 +63,7 @@ def create_dummy_course(self, store_type):
)
return course.id

def check_export_file(self, tar_file, course_key):
def check_export_file(self, tar_file, course_key, withTags=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use snake case: with_tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here

@@ -343,6 +344,11 @@ def export_handler(request, course_key_string):
requested_format = request.GET.get('_accept', request.META.get('HTTP_ACCEPT', 'text/html'))

if request.method == 'POST':
if not has_view_object_tags_access(request.user, str(course_key)):
raise PermissionDenied(
"You do not have permission to view object tags for this object_id."
Copy link
Contributor

Choose a reason for hiding this comment

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

We're already checking if not has_course_author_access(request.user, course_key): - isn't that enough?

I'm just slightly worried that this will cause some issue where exports start to fail for people who aren't even using tags at all in their course, so I want to play it safe here.

If you want to keep it in, we should just disable the tags.csv file when the user doesn't have permission, rather than making the whole export fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're already checking if not has_course_author_access(request.user, course_key): - isn't that enough?

I think you are right, I will follow this. 👍

If you want to keep it in, we should just disable the tags.csv file when the user doesn't have permission, rather than making the whole export fail.

I think do this will do the code more complex

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, just take it out for now. The existing check for has_course_author_access is sufficient. If you're allowed to export the course, you're allowed to see all the tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here

xmodule/modulestore/xml_importer.py Outdated Show resolved Hide resolved
@ChrisChV
Copy link
Contributor Author

ChrisChV commented Apr 8, 2024

@bradenmacdonald I fixed the nits. It's ready for merge

@bradenmacdonald
Copy link
Contributor

@ChrisChV There is just a minor pylint issue - please fix :)

@bradenmacdonald bradenmacdonald merged commit ddb407a into openedx:master Apr 8, 2024
67 checks passed
@openedx-webhooks
Copy link

@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.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

KyryloKireiev pushed a commit to raccoongang/edx-platform that referenced this pull request Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Tagging] Handle tags when importing/exporting courses
5 participants