Tag in Collections [FC-0062] by ChrisChV · Pull Request #35383 · openedx/openedx-platform

ChrisChV · 2024-08-28T00:09:26Z

Description

Updates tagging_api to enable tags objects with LibCollectionKey as key.

Supporting information

Github Issue: [Libraries] [Collections] [Backend] Add tags to collections modular-learning#228
Depends on: feat: LibraryCollectionKey and LibraryCollectionLocator created [FC-0062] opaque-keys#335
Internal ticket: https://tasks.opencraft.com/browse/FAL-3785

Testing instructions

Check all the code
Verify that the test covers the changes

Other information

Before merge: Bump version of edx-opaque-keys

openedx-webhooks · 2024-08-28T00:09:30Z

Thanks for the pull request, @ChrisChV!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

ChrisChV · 2024-08-28T00:10:45Z

 edx-milestones
 edx-name-affirmation
-edx-opaque-keys
+git+https://github.com/open-craft/opaque-keys.git@chris/FAL-3785-collections-key#egg=edx-opaque-keys


TODO: Bump opaque-keys version

rpenido

Thank you for your work @ChrisChV ! 👍

Could you add some tests for the view to check the permissions?

From the code, I think they will just work but I think it is worth the extra tests.

rpenido · 2024-08-28T17:55:42Z

+                try:
+                    return LibCollectionKey.from_string(key_str)
+                except InvalidKeyError as usage_key_error:
+                    raise ValueError("object_id must be a CourseKey, LibraryLocatorV2 or a UsageKey") from usage_key_error


nit:

Suggested change

raise ValueError("object_id must be a CourseKey, LibraryLocatorV2 or a UsageKey") from usage_key_error

raise ValueError("object_id must be one of the following keys: CourseKey, LibraryLocatorV2, UsageKey or LibCollectionKey") from usage_key_error

Updated here: 2849d4b

ChrisChV · 2024-08-30T03:06:42Z

@rpenido Thanks for the review.

Could you add some tests for the view to check the permissions?

I'm unsure how to test that at the Python API level, as that has not been checked there. As far as I understand that permission is checked in openedx-learning rest_api, just like all other object types.

pomegranited

👍 Looks good thank you @ChrisChV !

I tested this by tagging some collections from the command line.
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation
User-facing strings are extracted for translation

pomegranited · 2024-08-30T05:12:43Z


 def build_object_tree_with_objecttags(
-    content_key: LibraryLocatorV2 | CourseKey,
+    content_key: LibraryLocatorV2 | CourseKey | LibraryCollectionKey,


Is it ok to use content_tagging.types.ContentKey here?

Updated: 4bc07a0

rpenido · 2024-08-30T13:36:46Z

I'm unsure how to test that at the Python API level, as that has not been checked there. As far as I understand that, permission is checked in openedx-learning rest_api, just like all other object types.

It is a bit of back-and-forth. The permission is checked in openedx-learning, but we override the permission rules here.

You updated this file, so I think the permission was updated.

Here we test tagging a course, a block and a library. I think we should also test tagging a collection.

rpenido

LGTM 👍
Thank you @ChrisChV!

I tested this using the instructions from the PR
I read through the code
~~I checked for accessibility issues~~
Includes documentation

edx-pipeline-bot · 2024-09-13T16:29:04Z

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

edx-pipeline-bot · 2024-09-13T17:18:14Z

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

edx-pipeline-bot · 2024-09-13T17:18:14Z

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

edx-pipeline-bot · 2024-09-13T17:23:37Z

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

edx-pipeline-bot · 2024-09-13T17:41:33Z

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

ChrisChV added 2 commits August 27, 2024 17:21

chore: Use opencraft branch of opaque-keys

2765132

feat: Allow tag collections

b2e9ca7

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 28, 2024

ChrisChV changed the title ~~Chris/fal 3785 tags to collections~~ Tag in Collections [FC-0062] Aug 28, 2024

ChrisChV commented Aug 28, 2024

View reviewed changes

ChrisChV mentioned this pull request Aug 28, 2024

feat: LibraryCollectionKey and LibraryCollectionLocator created [FC-0062] openedx/opaque-keys#335

Merged

style: Fix lint

98ffb23

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Aug 28, 2024

rpenido suggested changes Aug 28, 2024

View reviewed changes

refactor: Update LibCollectionKey to LibraryCollectionKey

2849d4b

fix: Type check fails

af1665b

pomegranited approved these changes Aug 30, 2024

View reviewed changes

ChrisChV added 3 commits August 31, 2024 00:06

style: Clean code in get_context_key_from_key

5eac513

test: Check permissions for tag collections

278ab45

refactor: Verify LibraryCollectionKey in generate_csv_rows

4bc07a0

rpenido approved these changes Sep 2, 2024

View reviewed changes

ChrisChV added 6 commits September 4, 2024 09:11

Merge branch 'master' into chris/FAL-3785-tags-to-collections

55185d5

Merge branch 'master' into chris/FAL-3785-tags-to-collections

b6e3ee3

chore: Bump version of opaque-keys

c01c51f

chore: Bumpb version of opaque-keys

120900d

Merge branch 'master' into chris/FAL-3785-tags-to-collections

5554b9b

Merge branch 'master' into chris/FAL-3785-tags-to-collections

44bb7bb

ChrisChV merged commit dd59dc6 into openedx:master Sep 13, 2024

ChrisChV deleted the chris/FAL-3785-tags-to-collections branch September 13, 2024 14:19

ormsbee mentioned this pull request Sep 27, 2024

XBlock asset support for new libraries experience (WIP) #35557

Closed

	raise ValueError("object_id must be a CourseKey, LibraryLocatorV2 or a UsageKey") from usage_key_error
	raise ValueError("object_id must be one of the following keys: CourseKey, LibraryLocatorV2, UsageKey or LibCollectionKey") from usage_key_error

Uh oh!

Conversation

ChrisChV commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Uh oh!

openedx-webhooks commented Aug 28, 2024

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Uh oh!

ChrisChV Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisChV Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

rpenido left a comment

Choose a reason for hiding this comment

Uh oh!

rpenido Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisChV Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisChV commented Aug 30, 2024

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

pomegranited Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisChV Aug 31, 2024

Choose a reason for hiding this comment

Uh oh!

rpenido commented Aug 30, 2024

Uh oh!

rpenido left a comment

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Sep 13, 2024

Uh oh!

edx-pipeline-bot commented Sep 13, 2024

Uh oh!

edx-pipeline-bot commented Sep 13, 2024

Uh oh!

edx-pipeline-bot commented Sep 13, 2024

Uh oh!

edx-pipeline-bot commented Sep 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ChrisChV commented Aug 28, 2024 •

edited

Loading