Use object permissions in Tagging frontend [FC-0036] #787

pomegranited · 2024-01-10T06:27:28Z

Description

Uses the permissions added to the Tagging REST API by openedx/openedx-learning#138 to decide what actions (e.g. import, export, edit, delete) to present to the current user when viewing Tagging-related content.

Supporting information

Part of openedx/modular-learning#160

This PR is dependent on these PRs:

Testing instructions

Setup:

Run edx-platform from the branch on Tagging: serialize object permissions to REST API [FC-0036] edx-platform#34004
Install the updated requirements from ^.
Create test taxonomies, tags, orgs, and courses with https://github.com/open-craft/taxonomy-sample-data/
Run this PR's branch for course authoring.

Testing Tag Drawer:

Login to Studio as a non-superuser with course authoring permissions.
Visit the Sample Taxonomy Course course-v1:SampleTaxonomyOrg1+STC1+2023_1
Navigate to a Unit
Click "Manage Tags", either from the sidebar or the unit's menu
Ensure that the tag drawer opens with:
- All the taxonomies the user is allowed to view
- "Add tags" buttons for the listed taxonomies
- Any selected object tags shown.
Add a bunch of tags at various hierarchy levels, and ensure they add as expected.
Delete some tags and ensure they delete as expected.

Testing Taxonomy Manager:

Login to Studio as a non-superuser with course authoring permissions.
(Superusers have permission to do anything, and so aren't a good test of this change.)
Visit the Taxonomies list (http://localhost:2001/taxonomies)
Check that the "Import" button is enabled (new taxonomies can be added)
For each taxonomy listed, check that:
- All taxonomies show "Export" as a menu option
- Non-system taxonomies show "Re-import", "Manage organizations", and "Delete" as menu options
- System taxonomies do not show "Re-import" or "Delete" as menu options

Deadline

None

Private ref: FAL-3577

openedx-webhooks · 2024-01-10T06:27:32Z

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

to determine which actions/menu items to offer the user

codecov · 2024-01-10T06:56:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bdb4ffe) 89.95% compared to head (0eb4661) 90.30%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
+ Coverage   89.95%   90.30%   +0.35%     
==========================================
  Files         506      506              
  Lines        8450     8477      +27     
  Branches     1779     1784       +5     
==========================================
+ Hits         7601     7655      +54     
+ Misses        819      794      -25     
+ Partials       30       28       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…gging REST API to decide what actions to offer the user. These object-specific permissions replace the global "editable" field, which has been removed from the REST API.

to alleviate confusion about what permissions they reflect. Also fixes some properties and wraps menu clicks in act() to remove errors from the tests.

rpenido

Hi @pomegranited!
I think we mixed the Tag with ObjectTag permissions here. Instead of using the Tag canAdd and canDelete, we should use the Taxonomy canTagObject you created.

We will use the Tag permissions to manage the Tags of a Taxonomy (not to tag objects). Does this make sense?

src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx

src/content-tags-drawer/ContentTagsTree.jsx

src/content-tags-drawer/data/api.js

rpenido · 2024-01-15T21:31:57Z

src/taxonomy/TaxonomyListPage.jsx

@@ -157,6 +158,7 @@ const TaxonomyListPage = () => {
    return { taxonomyListData, isLoaded };
  };
  const { taxonomyListData, isLoaded } = useTaxonomyListData();
+  const canAdd = isLoaded ? taxonomyListData.canAdd : false;


Same. I think we need the canTagObject flag here

I disagree -- this is taxonomy.canAdd , which indicates whether the user can create a new taxonomy.

But clearly these permissions are confusing, so I can rename them to include the model name:

objecttag.canAdd -> canTagObjects (already done)

objecttag.canDelete -> canDeleteObjectTag

taxonomy.canAdd -> canAddTaxonomy

tag.canChange -> canChangeTag

tag.canDelete -> canDeleteTag

My bad here @pomegranited! You're right. I meant to mark the canAdd from ObjectTag.

No worries at all @rpenido ! It really helped me to hear your perspective reviewing these PRs, and I hope the code is now more legible to everyone.

rpenido

Good work @pomegranited! 👍

I tested this using the Testing Instructions from Use object permissions in Tagging frontend [FC-0036] #787
I read through the code
~~I checked for accessibility issues~~
Includes documentation
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx

Permissions are included in the response; no need to request them.

to avoid duplication.

xitij2000

Just minor suggestions, but looks good otherwise.

I think this will require the other PRs to merge first otherwise the UI might break. Is that correct?

xitij2000 · 2024-01-23T08:24:23Z

src/content-tags-drawer/TagBubble.test.jsx

+      <TagBubbleComponent
+        value={tagBubbleData.value}
+        lineage={data.lineage}
+        implicit={tagBubbleData.implicit}
+        removeTagHandler={tagBubbleData.removeTagHandler}
+      />,


I think for symmetry with the previous test, canRemove should be passed here as well.

xitij2000 · 2024-01-23T08:42:45Z

src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx

+    // Click on button again to close the menu
+    fireEvent.click(getByTestId('taxonomy-menu-button'));
+
+    // Menu closed
+    // Jest bug: toBeVisible() isn't checking opacity correctly
+    // expect(getByTestId('taxonomy-menu')).not.toBeVisible();
+    expect(getByTestId('taxonomy-menu').style.opacity).toEqual('0');
+
+    // Menu button still visible
+    expect(getByTestId('taxonomy-menu-button')).toBeVisible();


I think this part of the test can just be skipped. It's not testing functionality of this app, but testing that the dropdown component functions properly, which is (or should be) covered by paragon tests.

xitij2000 · 2024-01-23T09:07:10Z

src/content-tags-drawer/ContentTagsCollapsible.jsx

      value: PropTypes.string,
      lineage: PropTypes.arrayOf(PropTypes.string),
    })),
+    canTagObject: PropTypes.bool.isRequired,
  }).isRequired,
-  editable: PropTypes.bool.isRequired,
 };


Some of these type definition are repeated in multiple places, it might be worth defining them once and them reusing them.

i.e. you could do put into a data/proptypes.js the following:

import PropTypes from 'prop-types'; const tag = PropTypes.shape({ value: PropTypes.string, lineage: PropTypes.arrayOf(PropTypes.string), }); const taxonomy = PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, contentTags: PropTypes.arrayOf(tag), canTagObject: PropTypes.bool.isRequired, }); export default { tag, taxonomy, };

Then they can be used anywhere as:

import TaxonomyProps from './data/proptypes'; ContentTagsCollapsible.propTypes = { contentId: PropTypes.string.isRequired, taxonomyAndTagsData: TaxonomyProps.taxonomy.isRequired, };

This will avoid needing to update all places when the structure changes, and also will also make it easier to discover all the places that rely on this data structure.

Thank you for your review @xitij2000 ! And yep, we have to wait for openedx/edx-platform#34004 to merge before merging here.

Some of these type definition are repeated in multiple places, it might be worth defining them once and them reusing them.

Unfortunately, it looks like the components in src/taxonomy and src/content-tags-drawer don't share enough propTypes to warrant creating a prototypes.js file for them to use.

However, I do think it's silly that our test modules don't just re-use the propTypes of the components they're testing, so I've fixed that: 0eb4661
Let me know what you think?

Sure, I think that makes sense. I don't see any benefit in duplicating this, since it's already a development/testing tool more than a production tool.

pomegranited · 2024-01-25T01:46:30Z

FYI @xitij2000 openedx/edx-platform#34004 (review) should merge tomorrow -- are you ok with merging this in the next couple days?

xitij2000 · 2024-01-25T06:22:46Z

FYI @xitij2000 openedx/edx-platform#34004 (review) should merge tomorrow -- are you ok with merging this in the next couple days?

I'm on leave tomorrow, and then it's the weekend. Since merging is the only thing left, I can do that via my phone though.

pomegranited · 2024-01-25T23:12:08Z

@xitij2000 If you feel comfortable with merging on your day off, then that's fine. but there's no all-fired rush :) OK to wait until you're back at work next week.
Thank you!

openedx-webhooks · 2024-01-29T09:17:35Z

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

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

pomegranited marked this pull request as draft January 10, 2024 06:27

pomegranited force-pushed the jill/tagging-rest-perms branch from 917874e to 9deffad Compare January 10, 2024 06:32

feat: use the permissions returned by the taxonomy REST API

18a2aa3

to determine which actions/menu items to offer the user

pomegranited force-pushed the jill/tagging-rest-perms branch from 9deffad to 18a2aa3 Compare January 10, 2024 06:47

pomegranited added 5 commits January 11, 2024 15:58

fix: REST API is now returning flattened user permissions

cb2d814

Merge branch 'master' into jill/tagging-rest-perms

f6a3487

feat: Content Tags Drawer uses permissions returned by the content ta…

93b0ad0

…gging REST API to decide what actions to offer the user. These object-specific permissions replace the global "editable" field, which has been removed from the REST API.

fix: renames the can<Action> fields to can<Action><Modelname>

3b97b18

to alleviate confusion about what permissions they reflect. Also fixes some properties and wraps menu clicks in act() to remove errors from the tests.

Merge branch 'master' into jill/tagging-rest-perms

af8b611

pomegranited force-pushed the jill/tagging-rest-perms branch from 76277e6 to 93b0ad0 Compare January 15, 2024 05:00

pomegranited marked this pull request as ready for review January 15, 2024 06:23

This was referenced Jan 15, 2024

Tagging: serialize object permissions to REST API [FC-0036] openedx/openedx-learning#138

Merged

Tagging: serialize object permissions to REST API [FC-0036] openedx/edx-platform#34004

Merged

pomegranited requested a review from rpenido January 15, 2024 06:40

rpenido reviewed Jan 15, 2024

View reviewed changes

pomegranited force-pushed the jill/tagging-rest-perms branch from d5456b1 to 3b97b18 Compare January 16, 2024 02:46

rpenido self-requested a review January 17, 2024 13:51

rpenido approved these changes Jan 17, 2024

View reviewed changes

src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx Outdated Show resolved Hide resolved

fix: use await findByTestId instead of wrapping menu clicks in act()

5a1de30

pomegranited requested a review from xitij2000 January 18, 2024 22:54

pomegranited added 2 commits January 23, 2024 12:34

revert: removes include_perms from the tagging REST API calls

1348604

Permissions are included in the response; no need to request them.

refactor: use component PropTypes in tagging tests

0eb4661

to avoid duplication.

xitij2000 approved these changes Jan 23, 2024

View reviewed changes

pomegranited force-pushed the jill/tagging-rest-perms branch from 172add8 to 0eb4661 Compare January 24, 2024 01:24

xitij2000 merged commit b56b5d9 into openedx:master Jan 29, 2024
6 checks passed

xitij2000 deleted the jill/tagging-rest-perms branch January 29, 2024 09:17

pomegranited mentioned this pull request Feb 1, 2024

fix: fixed typo in updateContentTaxonomyTags URL [FC-0036] #815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use object permissions in Tagging frontend [FC-0036] #787

Use object permissions in Tagging frontend [FC-0036] #787

pomegranited commented Jan 10, 2024 •

edited

Loading

openedx-webhooks commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

rpenido left a comment

rpenido Jan 15, 2024

pomegranited Jan 15, 2024 •

edited

Loading

rpenido Jan 16, 2024

pomegranited Jan 17, 2024

rpenido left a comment

xitij2000 left a comment

xitij2000 Jan 23, 2024

xitij2000 Jan 23, 2024

xitij2000 Jan 23, 2024

pomegranited Jan 24, 2024 •

edited

Loading

xitij2000 Jan 24, 2024 •

edited

Loading

pomegranited commented Jan 25, 2024

xitij2000 commented Jan 25, 2024

pomegranited commented Jan 25, 2024

openedx-webhooks commented Jan 29, 2024

Use object permissions in Tagging frontend [FC-0036] #787

Use object permissions in Tagging frontend [FC-0036] #787

Conversation

pomegranited commented Jan 10, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Jan 10, 2024 • edited Loading

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

rpenido left a comment

Choose a reason for hiding this comment

rpenido Jan 15, 2024

Choose a reason for hiding this comment

pomegranited Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

rpenido Jan 16, 2024

Choose a reason for hiding this comment

pomegranited Jan 17, 2024

Choose a reason for hiding this comment

rpenido left a comment

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 Jan 23, 2024

Choose a reason for hiding this comment

xitij2000 Jan 23, 2024

Choose a reason for hiding this comment

xitij2000 Jan 23, 2024

Choose a reason for hiding this comment

pomegranited Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

xitij2000 Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited commented Jan 25, 2024

xitij2000 commented Jan 25, 2024

pomegranited commented Jan 25, 2024

openedx-webhooks commented Jan 29, 2024

pomegranited commented Jan 10, 2024 •

edited

Loading

openedx-webhooks commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

pomegranited Jan 15, 2024 •

edited

Loading

pomegranited Jan 24, 2024 •

edited

Loading

xitij2000 Jan 24, 2024 •

edited

Loading