feat: add update taxonomy orgs rest api [FC-0036] #33611
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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:
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. |
rpenido
force-pushed
the
rpenido/fal-3542-rest-api-to-assign-a-taxonomy-to-organizations
branch
3 times, most recently
from
a2f5f14 to
a1a86b1
Compare
rpenido
force-pushed
the
rpenido/fal-3542-rest-api-to-assign-a-taxonomy-to-organizations
branch
from
a1a86b1 to
a0f9e66
Compare
There was a problem hiding this comment.
Looks good @rpenido ! I've left some questions about the tests, but let me know if you disagree.
|
|
||
| @ddt.data( | ||
| (True, ["orgX"], "Using both all_orgs and orgs parameters should throw error"), | ||
| (False, None, "Using neither all_orgs or orgs parameter should throw error"), |
There was a problem hiding this comment.
This case looks valid to me.. How do we toggle from all_orgs = True to all_orgs = False?
There was a problem hiding this comment.
You must pass the all_orgs=False and an empty orgs (orgs=[]) array. If you think that omitting orgs should have the same effect, let me know!
Personally, I prefer the explicitly orgs=[] way.
openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py
Outdated
Show resolved
Hide resolved
…nomy-to-organizations
|
Sandbox deployment successful. Sandbox LMS is available at pr-33611-139931.staging.do.opencraft.hosting |
|
Sandbox update request received. Deployment will start soon. |
|
Sandbox deployment started. |
rpenido
force-pushed
the
rpenido/fal-3542-rest-api-to-assign-a-taxonomy-to-organizations
branch
from
d0fc470 to
243585a
Compare
|
Sandbox update request received. Deployment will start soon. |
|
Sandbox deployment started. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33611-139931.staging.do.opencraft.hosting |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33611-139931.staging.do.opencraft.hosting |
|
If @pomegranited is happy with this, I'll approve and merge on Monday. |
| # Check that the orgs didn't change | ||
| url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) | ||
| response = self.client.get(url) | ||
| assert response.data["orgs"] == [None] |
There was a problem hiding this comment.
This doesn't have to block merging, but I don't think it's very nice to send ["orgs"] == [None] to the frontend.
We in the backend know this means "all orgs", but the frontend shouldn't have to know this. I'd rather we sent an explicit "all_orgs" flag with the response to indicate this case.
There was a problem hiding this comment.
Great suggestion @pomegranited!
Done here: e2e8f76
bradenmacdonald
changed the title
bradenmacdonald
deleted the
rpenido/fal-3542-rest-api-to-assign-a-taxonomy-to-organizations
branch
|
@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. |
|
Sandbox update request received. Deployment will start soon. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Add a rest api endpoint to allow taxonomy admins to change the orgs that are linked to a taxonomy
This also adds the orgs from a taxonomy in the serializer used in the
listanddetailactions.Supporting Information
Testing instructions
Private-ref: FAL-3542