Fal-3449 [WIP] #552
Fal-3449 [WIP] #552
Conversation
rpenido
force-pushed
the
rpenido/fal-3449-taxonomy-view-management-apis
branch
from
f151c80
to
001334a
Compare
pomegranited
force-pushed
the
jill/add-content-tagging-smart-objecttags
branch
from
d780cc7
to
180a29c
Compare
There was a problem hiding this comment.
@rpenido I've left some suggestions here, and one feature request ("list taxonomies for org").
Feel free to push back if there's anywhere you disagree?
| username="staff", | ||
| email="[email protected]", | ||
| is_staff=True, | ||
| ) |
There was a problem hiding this comment.
The only thing that we need to test for content_tagging vs oel_tagging are the "org" membership allowance for normal users, so we need to see the view permission checks here done with the various org membership options.
cf test_rules.py -- I recommend splitting out the setUp method in TestRulesTaxonomy into a Mixin you can use here, so you can borrow the user- and org- creation logic there.
But we'll need to see the view permissions checks done with those users, and with ENABLE_CREATOR_GROUP turned on and turned off.
And please apply similar suggestions to your view tests here as I requested for https://github.com/open-craft/openedx-learning/pull/4/files/a0fedd36bbe06ef0a4c00466c8c9570f3c35616d#diff-2f11bfdf39bf0bee3b4778c0d84478fa6afd170e57245f804c77b2e9b4d865a4.
There was a problem hiding this comment.
Understood! This PR is still a WIP: I must create the tests/view related to the Org permissions.
Today I worked on the permissions here.
openedx/features/content_tagging/rest_api/v1/tests/test_views.py
Outdated
Show resolved
Hide resolved
openedx/features/content_tagging/rest_api/v1/tests/test_views.py
Outdated
Show resolved
Hide resolved
| update_org_role(self.staff, OrgContentCreatorRole, self.user, [self.org]) | ||
|
|
||
| @ddt.data( | ||
| ( |
There was a problem hiding this comment.
@pomegranited
This isn't looking good.
At first, I tried to have a dict for each test, but it became HUGE (and messy to read). For each method (CREATE, UPDATE, PATCH, DETAIL, DELETE), we have 4 combinations of DISABLED_COURSE_CREATION and ENABLED_CREATOR_GROUP flags and 5 (maybe 1 more for an Org Member User) resulting in 20-24 test cases. I tried consolidating some tests but am not satisfied with the result: we cannot verify which case fails because now we only have 4 test cases.
Is there a less verbose way to make the combinations of users x flags to have all 20-24 test cases separately? Use a file?
I'm thinking of subtests, but maybe you have a better idea!
There was a problem hiding this comment.
Yeah, it's a lot.. and we do want our tests to remain as readable as possible.
For the ENABLES_CREATOR_GROUP flag, I did this:
- Pulled common setUp data and utility methods into a TestTaxonomyMixin.
- Wrote my "normal situation" tests with TestRulesTaxonomy, which [subclasses TestTaxonomyMixin and enables
ENABLED_CREATOR_GROUP. - Wrote my "nobody gets to do anything" tests using a subclass of TestRulesTaxonomy, which disables
ENABLED_CREATOR_GROUPand overrides any changed tests that are in TestRulesTaxonomy.
Pylint doesn't like this (so you have to quiet it), and you also need to be careful with ddt, and make sure your overridden test methods have the exact same calling data as the parent class.
openedx/features/content_tagging/rest_api/v1/tests/test_views.py
Outdated
Show resolved
Hide resolved
pomegranited
force-pushed
the
jill/add-content-tagging-smart-objecttags
branch
2 times, most recently
from
1a3c38f
to
6206316
Compare
| @@ -80,6 +81,77 @@ def get_taxonomies_for_org( | |||
| yield taxonomy.cast() | |||
|
|
|||
|
|
|||
| # Experiment | |||
| def get_taxonomies_for_org_queryset( | |||
There was a problem hiding this comment.
FYI @rpenido -- you don't have to write this part anymore, we've done it here: https://github.com/openedx/edx-platform/pull/32661/files#diff-0ed9e2727b75df5366745749e3b39718ffbefe4b60289d80feab3b0019feb66bR81-R100
Things changed pretty substantially on that branch, so you may want to take a step back and re-read that base PR, and apply your changes to that instead of starting from here?
There was a problem hiding this comment.
Also fyi, openedx#32661 is merged (yay!), so you can rebase this on latest openedx:master and open a new PR against the upstream repo.
There was a problem hiding this comment.
Yay! \o/
Tomorrow I will rebase and create a new PR against upstream.
* fix: use student role for zoom in case of global staff. * fix: added request cache to avoid duplicate db calls.
This PR implements much of the static assets rework ADR [1], including: * `npm run build[-dev]`, and its subcommands, * `npm run webpack[-dev]` and * `npm run compile-sass[-dev]`. This is backwards-compatible. `paver update_assets` should not be affected. The new command warns that it is "experimental" for a few reasons: * `npm run build` will fail in the webpack phase unless you first run `xmodule_assets`. This will be changed soon [2]. * We have tested the new build, but not quite so thoroughly that we'd recommend it as the production default yet. Once the xmodule_assets work lands, we'll share this on the forums so early adopters can try it out. * The commands lack some top-level documentation. Once they stabilize more, we'll add a section to the README that explains how and when to use `npm run build` and its subcommands and its env vars. * `npm run watch` is not yet implemented. References: 1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst 2. openedx#32685 Part of: openedx#31604
rpenido
force-pushed
the
rpenido/fal-3449-taxonomy-view-management-apis
branch
from
c0c8e55
to
181cb57
Compare
* fix: allow missing authOrgId --------- Co-authored-by: Leangseu Kim <[email protected]>
Co-authored-by: Diego Velásquez <[email protected]>
…enedx#32480) As part of the static asset build, JS modules for most built-in XBlocks were unnecessarily copied from the original locations (under xmodule/js and common/static/js) to a git-ignored location (under common/static/xmodule), and then included into the Webpack builld via common/static/xmodule/webpack.xmodule.config.js. With this commit, we stop copying the JS modules. Instead, we have common/static/xmodule/webpack.xmodule.config.js just reference the original source under xmodule/js and common/static/js. This lets us us radically simplify the xmodule/static_content.py build script. It also sets the stage for the next change, in which we will check webpack.xmodule.config.js into the repository, and delete xmodule/static_content.py entirely. common/static/xmodule/webpack.xmodule.config.js before: module.exports = { "entry": { "AboutBlockDisplay": [ "./common/static/xmodule/modules/js/000-b82f6c436159f6bc7ca2513e29e82503.js", "./common/static/xmodule/modules/js/001-3ed86006526f75d6c844739193a84c11.js", "./common/static/xmodule/modules/js/002-3918b2d4f383c04fed8227cc9f523d6e.js", "./common/static/xmodule/modules/js/003-b3206f2283964743c4772b9d72c67d64.js", "./common/static/xmodule/modules/js/004-274b8109ca3426c2a6fde9ec2c56e969.js", "./common/static/xmodule/modules/js/005-26caba6f71877f63a7dd4f6796109bf6.js" ], "AboutBlockEditor": [ "./common/static/xmodule/descriptors/js/000-b82f6c436159f6bc7ca2513e29e82503.js", "./common/static/xmodule/descriptors/js/001-19c4723cecaa5a5a46b8566b3544e732.js" ], // etc } }; common/static/xmodule/webpack.xmodule.config.js after: module.exports = { "entry": { "AboutBlockDisplay": [ "./xmodule/js/src/xmodule.js", "./xmodule/js/src/html/display.js", "./xmodule/js/src/javascript_loader.js", "./xmodule/js/src/collapsible.js", "./xmodule/js/src/html/imageModal.js", "./xmodule/js/common_static/js/vendor/draggabilly.js" ], "AboutBlockEditor": [ "./xmodule/js/src/xmodule.js", "./xmodule/js/src/html/edit.js" ], // etc } }; Part of: openedx#32481
* refactor: moves is_content_creator from cms.djangoapps.contentstore.helpers to common.djangoapps.student.auth * feat: adds content tagging app Adds models and APIs to support tagging content objects (e.g. XBlocks, content libraries) by content authors. Content tags can be thought of as "name:value" fields, though underneath they are a bit more complicated. * adds dependency on openedx-learning<=0.1.0 * adds tagging app to LMS and CMS * adds content tagging models, api, rules, admin, and tests. * content taxonomies and tags can be maintained per organization by content creators for that organization.
openedx/features/content_tagging/rest_api/v1/tests/test_views.py
Outdated
Show resolved
Hide resolved
| if is_global_taxonomy_admin(self.request.user): | ||
| return taxonomies | ||
| else: | ||
| # This should be an optional parameter on get_taxonomies_for_org? | ||
| user_orgs = get_user_taxonomy_orgs(self.request.user) | ||
| user_orgs_short_name = [org.short_name for org in user_orgs] | ||
| return taxonomies.filter( | ||
| Q( | ||
| Exists( | ||
| TaxonomyOrg.objects.filter( | ||
| taxonomy=OuterRef("pk"), | ||
| rel_type=TaxonomyOrg.RelType.OWNER, | ||
| org__short_name__in=user_orgs_short_name, | ||
| ) | ||
| ) | ||
| ) | ||
| | Q(enabled=True) | ||
| ) |
There was a problem hiding this comment.
This code probably needs to go to the API.
get_taxonomies_for_org should have a parameter (from_orgs?) that receives a list of orgs to which the caller has permissions.
We need this to differentiate both user cases
("userA", None, "orgA", ("st1", "t1", "tA1", "tA2", "tC1", "tC2")) # The user can see "tA2" because he is from A
("userB", None, "orgA", ("st1", "t1", "tA1", "tC1", "tC2")) # "tA2" is filtered out because is disabledThere was a problem hiding this comment.
This code probably needs to go to the API.
Agreed.. shouldn't have complex model queries in the views here.
Is there a way to use the rules to filter these instead of reproducing all of that logic here? It'll mean writing your own "list taxonomies by org" viewset method, but it keeps the logic cleaner.
Something like:
if "org" in query_params:
taxonomies = get_taxonomies_for_org(enabled, org)
else:
taxonomies = get_taxonomies(enabled)
result = []
for taxonomy in taxonomies[paginated]:
if request.user.has_perm("oel_tagging.view_taxonomy", taxonomy):
result.append(taxonomy)
return Result(result)
There was a problem hiding this comment.
I don't think we will be able to. We want to filter before pagination (or we will end with "holes" in pages). Also, we don't want to iterate over the whole list before pagination (to know the record count, for example).
I will try to consolidate this and put it in rules.py as a sort of "taxonomy_list_filter". That way, we at least leave can_view_taxonomy and the list filter "together.
There was a problem hiding this comment.
The actual solution is better proposed here: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0009-bp-permissions.html
…2686) * fix: push docker multi-arch images --------- Co-authored-by: Salman Nawaz <[email protected]>
Co-authored-by: Diego Velásquez <[email protected]>
* fix: include new assets.txt file in make upgrade * test: update click version to resolve upgrade job failure * chore: Updating Python Requirements (openedx#32861)
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
…x#32863) Co-authored-by: adeel.tajamul <[email protected]>
…x-enterprise-34f9fc4 feat: Upgrade Python dependency edx-enterprise
…ts` (openedx#32685) The Webpack configuration file for built-in XBlock JS used to be generated at build time and git-ignored. It lived at common/static/xmodule/webpack.xmodule.config.js. It was generated because the JS that it referred to was also generated at build-time, and the filenames of those JS modules were not static. Now that its contents have been made entirely static [1], there is no reason we need to continue generating this Webpack configuration file. So, we check it into edx-platform under the name ./webpack.builtinblocks.config.js. We choose to put it in the repo's root directory because the paths contained in the config file are relative to the repo's root. This allows us to behead both the xmodule/static_content.py (`xmodule_assets`) script andthe `process_xmodule_assets` paver task, a major step in removing the need for Python in the edx-platform asset build [2]. It also allows us to delete the `HTMLSnippet` class and all associated attributes, which were exclusively used by xmodule/static_content.py.. We leave `xmodule_assets` and `process_xmodule_assets` in as stubs for now in order to avoid breaking external code (like Tutor) which calls Paver; the entire pavelib/assets.py function will be eventually removed soon anyway [3]. Further, to avoid extraneous refactoring, we keep one method of `HTMLSnippet` around on a few of its former subclasses: `get_html`. This method was originally part of the XModule framework; now, it is left over on a few classes as a simple internal helper method. References: 1. openedx#32480 2. openedx#31800 3. openedx#31895 Part of: openedx#32481
Implements the `npm run watch` section of the assets ADR [1], plus some modifications since I decided to switch from pywatchman to watchdog (see ADR changes for justification). This will replace `paver watch_assets` (edx-platform) and `openedx-assets watch-themes` (Tutor). Specifically, this PR adds three experimental commands: * `npm run watch-sass` : Watch for Sass changes with watchdog. * `npm run watch-webpack` : Invoke Webpack-watch for JS changes. * `npm run watch` : Invoke both `watch-sass` and `watch-webpack` simultaneously. These commands are only intended to work in development mode. They have been tested both on bare-metal edx-platform and through `tutor dev run` on on Linux. Before removing the "experimental" label, we need to: * Test through Devstack on Linux. * Test through Devstack and `tutor dev run` on macOS. * Test on bare-metal macOS. Might not work, which is OK, but we should document that. * Document the commands in edx-platform's README. * Confirm that this not only works through `tutor dev run`, but also as a suitable replacement in the `watchthemes` service that Tutor runs automatically as part of `tutor dev start`. Tweak if necessary. References: 1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst Part of: openedx#31612
Adds validation for the grading assignment and grading policy, showing a warning in Studio if there is a mismatch.
…on-bump chore: version bump
rpenido
force-pushed
the
rpenido/fal-3449-taxonomy-view-management-apis
branch
from
75f28c6
to
b6a9173
Compare
|
Closed in favor of openedx#32871 |
No description provided.