feat: add taxonomies for org api #32871

rpenido · 2023-07-27T20:50:32Z

Description

This PR adds support for creating, viewing, updating and deleting Taxonomies for Organizations via REST APIs, with the permissions applied accordingly.

Supporting information

Depends on Taxonomy view/management REST APIs openedx-learning#63
Closes [Tagging] Taxonomies for Org API modular-learning#74

Testing instructions

Check the unit tests in test_views.py and test_rules.py are correct.
Run the unit tests

Manual check can be done in the CMS App:

Run the cms app
Login using Django Admin (non-staff users can login, they just can't see Django Admin)
- http://127.0.0.1:8000/admin/
REST API is here:
- http://127.0.0.1:8000/api/content_tagging/v1/

Private-ref: FAL-3450

openedx/features/content_tagging/rest_api/v1/views.py

openedx/features/content_tagging/rules.py

rpenido · 2023-07-30T15:06:15Z

~~ToDo: Handle {"ENABLE_CREATOR_GROUP": False} when everything else is ok.~~
Done

pomegranited

Looking good @rpenido , especially the tests!
Let me know if you have any questions or objections to the changes I've suggested?

cms/urls.py

openedx/features/content_tagging/models.py

openedx/features/content_tagging/rest_api/v1/views.py

openedx/features/content_tagging/rest_api/v1/tests/test_views.py

requirements/edx/kernel.in

openedx/features/content_tagging/models.py

pomegranited · 2023-08-02T01:26:29Z

openedx/features/content_tagging/rest_api/v1/views.py

+        page_size = 100
+        max_page_size = 1000
+
+    pagination_class = TaxonomyPagination


I'm sorry, didn't notice that the pagination was only added here instead of on the openedx-learning side..
Do you mind to create a small PR against openedx-learning that moves this over there?

Done: openedx/openedx-learning#69

ToDo: remove pagination when the PR is merged:

feat: add taxonomy api pagination openedx-learning#69

Your PR is merged and now available with openedx-learning==0.1.2.

If you run make requirements in your LMS shell, that should bump the installed version in the requirements/edx/* files.
And if you run into this issue, see these comments on stackoverflow:

Exception: Can not find valid pkg-config name. Specify MYSQLCLIENT_CFLAGS and MYSQLCLIENT_LDFLAGS env vars manually

openedx/features/content_tagging/models.py

openedx/features/content_tagging/tests/test_rules.py

openedx/features/content_tagging/rules.py

openedx-webhooks · 2023-08-03T16:21:21Z

Thanks for the pull request, @rpenido! 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.

openedx/features/content_tagging/tests/test_rules.py

rpenido · 2023-08-04T22:56:29Z

Hi @pomegranited! I reverted the changes that are not related to the Taxonomy model.

pomegranited

Hi @rpenido , I've requested a few changes, and noted some things that I'd like to fix as part of a separate task.

I'm also awaiting word from Product on how they want the org content author permissions to work, something that came up in our last meeting. So this might sit in External Review for a little while before it can be merged.

But if you could address the requested changes, I'll work on the rest and let you know.

Thank you!

openedx/features/content_tagging/models/base.py

openedx/features/content_tagging/rest_api/v1/filters.py

openedx/features/content_tagging/rest_api/v1/views.py

openedx/features/content_tagging/rules.py

openedx/features/content_tagging/rest_api/v1/filters.py

openedx/features/content_tagging/rest_api/v1/tests/test_views.py

openedx/features/content_tagging/tests/test_rules.py

rpenido · 2023-08-08T20:37:06Z

Hi @pomegranited!

I made the changes and also reverted some "style only" modifications to get a clear diff. Thank you for your input on that.

I will squash/rebase when we are ready to External Review!

rpenido · 2023-08-09T13:36:35Z

Hi @pomegranited !
The change in permissions requested is here: b10b02a2288145408c9e6614bdf0d719c1f37f3f (including fixing the view tests and cleaning some code)
I cherry-picked the new test_rules.py and fixed it with the new conditions here: 3dccf60435e94d9b4ddf16d2f6b300866a690269

- [ ] ToDo: change the test_views to use helper functions
Is the proposal to make a helper function that checks list, get, edit, update, delete (for taxonomy, tag, objecttag), and then duplicate this function for each "user role"?
We will end up with multiple methods and views in the same test. I'm not sure about this one in the test_view context. Do you think this is the way to go?

pomegranited · 2023-08-09T23:50:45Z

openedx/features/content_tagging/rest_api/v1/filters.py

+        return queryset.filter(enabled=True).filter(
+            Q(
+                Exists(
+                    TaxonomyOrg.objects.filter(
+                        taxonomy=OuterRef("pk"),
+                        rel_type=TaxonomyOrg.RelType.OWNER,
+                        org=None,
+                    )
+                )
+            )
+            | Q(
+                Exists(
+                    TaxonomyOrg.objects.filter(
+                        taxonomy=OuterRef("pk"),
+                        rel_type=TaxonomyOrg.RelType.OWNER,
+                        org__short_name__in=[
+                            org.short_name
+                            for org in Organization.objects.all()
+                            if is_taxonomy_user_for_org(request.user, org)
+                        ],
+                    )
+                )
+            )
+        )


We only need the enabled filter here -- the rest of this org checking can go.

Suggested change

return queryset.filter(enabled=True).filter(

Q(

Exists(

TaxonomyOrg.objects.filter(

taxonomy=OuterRef("pk"),

rel_type=TaxonomyOrg.RelType.OWNER,

org=None,

)

)

)

| Q(

Exists(

TaxonomyOrg.objects.filter(

taxonomy=OuterRef("pk"),

rel_type=TaxonomyOrg.RelType.OWNER,

org__short_name__in=[

org.short_name

for org in Organization.objects.all()

if is_taxonomy_user_for_org(request.user, org)

],

)

)

)

)

return queryset.filter(enabled=True)

pomegranited · 2023-08-09T23:53:06Z

openedx/features/content_tagging/rules.py

+    Taxonomy users include global staff and superusers, course creators who can create courses for any org and
+    content creators for this specific org.
+    """
+    return is_content_creator(user, org.short_name)


As noted, we don't need any of this "is_content_creator" logic anymore. Only global staff + superusers can add/edit taxonomies, and anyone can view an enabled taxonomy, no matter its org.

bradenmacdonald · 2023-08-21T19:59:12Z

@rpenido Please fix the failing test and then I can merge this.

rpenido · 2023-08-22T17:25:28Z

Hi @bradenmacdonald! Do you know how to make the tests run again here?

bradenmacdonald · 2023-08-22T17:42:00Z

@rpenido They seem to be running now.

rpenido · 2023-08-22T18:52:16Z

I think we are good now @bradenmacdonald! Thank you!

yusuf-musleh · 2023-08-22T19:19:41Z

@bradenmacdonald I just rebased my PR open-craft#577 on @rpenido's work here. Was wondering if you'd like to take a take a look at it and merge it into this branch before merging this one in to master? Or are we better off doing it separately? The related openedx-learning PR openedx/openedx-learning#68 was already merged.

bradenmacdonald · 2023-08-22T20:22:30Z

@yusuf-musleh If it's just those two lines of code that were already approved, please go ahead and merge into this branch. Hope that's OK with you @rpenido.

rpenido · 2023-08-22T20:32:55Z

Of course @bradenmacdonald and @yusuf-musleh! Let's reduce the overhead and earn some time!

yusuf-musleh · 2023-08-22T20:36:08Z

@bradenmacdonald @rpenido Sounds good, I just need to bump up the installed version of the openedx-learning package. I'm currently running make upgrade package=openedx-learning locally to get the updated requirement files, will push it to my PR and merge once I confirm the tests pass (they are failing because of that),

yusuf-musleh · 2023-08-22T22:17:47Z

@bradenmacdonald Had to make a small change to the on the python API side and update the relevant tests. This is after the openedx-learning version upgrade. Tests should be fixed now, waiting for them to complete, then will merge, unless you have any issues with the additional changes I added.

…apis

rpenido · 2023-08-23T14:32:18Z

requirements/constraints.txt

-# openedx-learning new version has some changes which are breaking quality tests
-# See https://github.com/openedx/openedx-learning/pull/68 for the changes.
-# It needs to be updated in a separate issue.
-openedx-learning==0.1.2


@bradenmacdonald I removed this to make the tests pass. I'm not sure what "breaking quality tests" means here.

@rpenido I believe that was because when my changes were merged to the openedx-learning repo, it contained changes that remove some kwargs from some functions that were being called in this repo, and that would cause the quality tests to fail (hence the version was pinned to not use the latest). Now that everything is being merged, this change should be fine, in fact, it is needed.

Thank you @yusuf-musleh!

rpenido · 2023-08-24T12:33:58Z

Hi @bradenmacdonald!
Is there something we can do to help this get merged?

Thank you!

bradenmacdonald · 2023-08-24T17:01:27Z

@rpenido Thanks for the ping; I actually didn't realize this was waiting for me. I'll merge it in 30 min.

openedx-webhooks · 2023-08-24T17:32:12Z

@rpenido 🎉 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 · 2023-08-24T18:56:00Z

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

edx-pipeline-bot · 2023-08-24T19:18:39Z

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

edx-pipeline-bot · 2023-08-24T19:44:36Z

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

rpenido mentioned this pull request Jul 27, 2023

Fal-3449 [WIP] open-craft/edx-platform#552

Closed

rpenido commented Jul 30, 2023

View reviewed changes

openedx/features/content_tagging/rest_api/v1/views.py Outdated Show resolved Hide resolved

rpenido commented Jul 30, 2023

View reviewed changes

openedx/features/content_tagging/rules.py Show resolved Hide resolved

rpenido commented Jul 30, 2023

View reviewed changes

openedx/features/content_tagging/rules.py Outdated Show resolved Hide resolved

pomegranited requested changes Aug 1, 2023

View reviewed changes

pomegranited reviewed Aug 1, 2023

View reviewed changes

openedx/features/content_tagging/rest_api/v1/tests/test_views.py Outdated Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 750daaa to 0071e91 Compare August 1, 2023 19:29

pomegranited reviewed Aug 1, 2023

View reviewed changes

requirements/edx/kernel.in Outdated Show resolved Hide resolved

openedx/features/content_tagging/models.py Outdated Show resolved Hide resolved

pomegranited mentioned this pull request Aug 2, 2023

[Tagging] Implement tagging REST API: Retrieve object tags openedx/modular-learning#81

Closed

pomegranited reviewed Aug 2, 2023

View reviewed changes

pomegranited mentioned this pull request Aug 2, 2023

[Tagging] Single taxonomy view API openedx/modular-learning#75

Closed

pomegranited reviewed Aug 3, 2023

View reviewed changes

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

rpenido commented Aug 4, 2023

View reviewed changes

openedx/features/content_tagging/tests/test_rules.py Show resolved Hide resolved

rpenido commented Aug 4, 2023

View reviewed changes

openedx/features/content_tagging/tests/test_rules.py Show resolved Hide resolved

rpenido commented Aug 4, 2023

View reviewed changes

openedx/features/content_tagging/tests/test_rules.py Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 97fe65b to ef81ea8 Compare August 4, 2023 22:29

pomegranited requested changes Aug 8, 2023

View reviewed changes

rpenido changed the title ~~feat: add taxonomies for org api [wip]~~ feat: add taxonomies for org api Aug 8, 2023

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch 2 times, most recently from 9f89936 to 36f8dcc Compare August 9, 2023 13:25

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 36f8dcc to 3dccf60 Compare August 9, 2023 13:41

pomegranited requested changes Aug 9, 2023

View reviewed changes

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch 2 times, most recently from 67ca1d8 to c81c7bd Compare August 10, 2023 16:24

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 55696a8 to 210aed9 Compare August 15, 2023 22:27

rpenido marked this pull request as ready for review August 18, 2023 21:16

bradenmacdonald approved these changes Aug 21, 2023

View reviewed changes

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch 2 times, most recently from 7ee6d0b to 3591589 Compare August 22, 2023 17:23

rpenido and others added 3 commits August 23, 2023 09:04

feat: add taxonomies for org api

751ac04

chore: run CI

2819efc

feat: Add retrieve object_tags REST API (#577)

a19b2ea

rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 07b8e9a to a19b2ea Compare August 23, 2023 12:07

rpenido added 2 commits August 23, 2023 10:05

chore: update requirements

d414cd0

Merge branch 'master' into rpenido/fal-3449-taxonomy-view-management-…

c087515

…apis

rpenido commented Aug 23, 2023

View reviewed changes

rpenido mentioned this pull request Aug 23, 2023

feat: add language auto-tagging #32907

Merged

bradenmacdonald merged commit ecc4a0d into openedx:master Aug 24, 2023
43 checks passed

bradenmacdonald deleted the rpenido/fal-3449-taxonomy-view-management-apis branch August 24, 2023 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add taxonomies for org api #32871

feat: add taxonomies for org api #32871

rpenido commented Jul 27, 2023 •

edited

Loading

rpenido commented Jul 30, 2023 •

edited

Loading

pomegranited left a comment

pomegranited Aug 2, 2023

rpenido Aug 2, 2023

rpenido Aug 3, 2023 •

edited

Loading

pomegranited Aug 14, 2023

openedx-webhooks commented Aug 3, 2023 •

edited

Loading

rpenido commented Aug 4, 2023

pomegranited left a comment

rpenido commented Aug 8, 2023 •

edited

Loading

rpenido commented Aug 9, 2023 •

edited

Loading

pomegranited Aug 9, 2023

pomegranited Aug 9, 2023

rpenido Aug 10, 2023

bradenmacdonald commented Aug 21, 2023

rpenido commented Aug 22, 2023

bradenmacdonald commented Aug 22, 2023

rpenido commented Aug 22, 2023

yusuf-musleh commented Aug 22, 2023

bradenmacdonald commented Aug 22, 2023

rpenido commented Aug 22, 2023

yusuf-musleh commented Aug 22, 2023 •

edited

Loading

yusuf-musleh commented Aug 22, 2023

rpenido Aug 23, 2023

yusuf-musleh Aug 23, 2023

rpenido Aug 23, 2023

rpenido commented Aug 24, 2023

bradenmacdonald commented Aug 24, 2023

openedx-webhooks commented Aug 24, 2023

edx-pipeline-bot commented Aug 24, 2023

edx-pipeline-bot commented Aug 24, 2023

edx-pipeline-bot commented Aug 24, 2023

feat: add taxonomies for org api #32871

feat: add taxonomies for org api #32871

Conversation

rpenido commented Jul 27, 2023 • edited Loading

Description

Supporting information

Testing instructions

rpenido commented Jul 30, 2023 • edited Loading

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Aug 3, 2023 • edited Loading

rpenido commented Aug 4, 2023

pomegranited left a comment

Choose a reason for hiding this comment

rpenido commented Aug 8, 2023 • edited Loading

rpenido commented Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Aug 21, 2023

rpenido commented Aug 22, 2023

bradenmacdonald commented Aug 22, 2023

rpenido commented Aug 22, 2023

yusuf-musleh commented Aug 22, 2023

bradenmacdonald commented Aug 22, 2023

rpenido commented Aug 22, 2023

yusuf-musleh commented Aug 22, 2023 • edited Loading

yusuf-musleh commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Aug 24, 2023

bradenmacdonald commented Aug 24, 2023

openedx-webhooks commented Aug 24, 2023

edx-pipeline-bot commented Aug 24, 2023

edx-pipeline-bot commented Aug 24, 2023

edx-pipeline-bot commented Aug 24, 2023

rpenido commented Jul 27, 2023 •

edited

Loading

rpenido commented Jul 30, 2023 •

edited

Loading

rpenido Aug 3, 2023 •

edited

Loading

openedx-webhooks commented Aug 3, 2023 •

edited

Loading

rpenido commented Aug 8, 2023 •

edited

Loading

rpenido commented Aug 9, 2023 •

edited

Loading

yusuf-musleh commented Aug 22, 2023 •

edited

Loading