feat: allow filtering library by publish status #1570

DanielVZ96 · 2024-12-13T22:03:18Z

Description

Adds a filter widget:

And an "Unpublished changes" badge:

Supporting information

Issue: Add publish status filter #1484

Testing instructions

First pull the following branch: openedx/edx-platform#36031

[If needed] Create a new library with new components in http://apps.local.openedx.io:2001/course-authoring/libraries
With the tutor stack running execute the following command: tutor dev exec cms bash -c './manage.py cms reindex_studio --experimental '
Publish and edit some components. Take note of the number of components that have been published, published and modified after, and never been published
Open the filter widget and assert that the numbers of each publish status corresponds to the number you wrote down in step 2
Assert that filtering by each value only shows the correct components:
- Published when the component was published and not modified since
- Modified when the component has been modified after being published
- Never when the component has never been published

Deadline

ASAP

Other information

Private-Ref

openedx-webhooks · 2024-12-13T22:03:22Z

Thanks for the pull request, @DanielVZ96!

This repository is currently maintained by @openedx/2u-tnl.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.

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.

codecov · 2024-12-14T22:38:49Z

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.96%. Comparing base (b110b6b) to head (6d3db8d).

Files with missing lines	Patch %	Lines
src/search-manager/FilterByPublished.tsx	91.30%	2 Missing ⚠️
src/search-manager/data/apiHooks.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1570      +/-   ##
==========================================
- Coverage   92.96%   92.96%   -0.01%     
==========================================
  Files        1075     1076       +1     
  Lines       21190    21227      +37     
  Branches     4554     4565      +11     
==========================================
+ Hits        19700    19734      +34     
+ Misses       1424     1421       -3     
- Partials       66       72       +6

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

pomegranited

Hi @DanielVZ96 , this is working well! Left a few comments about extracting out user-facing strings, and some UI questions/issues. Let me know when it's ready for another review?

pomegranited · 2024-12-17T01:33:59Z

src/library-authoring/components/BaseComponentCard.tsx

@@ -75,7 +79,8 @@ const BaseComponentCard = ({
            <div className="text-truncate h3 mt-2">
              <Highlight text={displayName} />
            </div>
-            <Highlight text={description} />
+            <Highlight text={description} /><br />
+            {props.hasUnpublishedChanges ? <Badge variant="warning">Unpublished changes</Badge> : null}


Please extract "Unpublished changes" into a message so it can be translated.

pomegranited · 2024-12-17T01:39:26Z

src/search-manager/FilterByPublished.tsx

+  return (
+    <SearchFilterWidget
+      appliedFilters={[]}
+      label="Publish Status"


Please extract "Publish Status" to a message so it can be translated.

pomegranited · 2024-12-17T01:40:36Z

src/search-manager/FilterByPublished.tsx

+
+  const clearFilters = React.useCallback(() => {
+    setPublishStatusFilter([]);
+  }, []);


Selecting a Published Status filter doesn't show the "clear filters" link?

Need to modify canClearFilters.

pomegranited · 2024-12-17T01:41:17Z

src/search-manager/FilterByPublished.tsx

+              onChange={() => { toggleFilterMode(PublishStatus.Published); }}
+            >
+              <div>
+                Published


Please extract these "Published", "Modified since publish", and "Never published" labels to messages so they can be translated.

pomegranited · 2024-12-17T01:47:04Z

src/search-manager/SearchManager.ts

@@ -163,6 +171,7 @@ export const SearchContextProvider: React.FC<{
    searchKeywords,


Some UI questions, feel free to bounce these off the UX folks if you agree :)

Currently, sorting by "Recently Published" also filters out any "Never published" items, but this is entirely hidden from the user.
Should we make this explicit force-selecting the "Published" and "Modified since publish" filters in the UI?

Should the Publish Status drop-down label reveal when some filters are selected, like the Type and Tags do?

To make the drop-down more concise, I'd use "Status: Published" or whatever instead of "Published status: Published".

@jmakowski1123 @lizc577 @sdaitzman @marcotuts any comments on this? ^

@jmakowski1123 @lizc577 @sdaitzman @marcotuts gentle nudge on this since we're blocked waiting for a decision here.

pomegranited · 2024-12-17T01:55:15Z

src/search-manager/FilterByPublished.tsx

+            >
+              <div>
+                Published
+                {' '}<Badge variant="light" pill>{publishStatus[PublishStatus.Published] ?? 0}</Badge>


I don't think you need to have this {' '} in there to separate the label from the Badge, do you? FilterByTags doesn't: https://github.com/open-craft/frontend-app-authoring/blob/dvz/filter-by-published/src/search-manager/FilterByTags.tsx#L66-L67

(And strictly speaking, the Badge should be part of the translated text so that RTL languages can put the count badge to the left of the text. But there's a number of places where we'd need to fix this, so it's ok if you don't do it here.)

jmakowski1123 · 2025-01-21T16:58:32Z

I'd go with force-suggesting the "only published" filter.

sdaitzman · 2025-01-23T18:47:03Z

Agreed! Also adding, I do think Publish Status drop-down label should reveal when some filters are selected, like the Type and Tags do

DanielVZ96 requested a review from a team as a code owner December 13, 2024 22:03

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

bradenmacdonald and others added 4 commits December 14, 2024 20:29

feat: allow filtering library by publish status

fd2c357

temp: more work toward working publish status filter

7cf5d2e

feat: implement pills and filter

8889017

fix: improve test coverage

6d3db8d

DanielVZ96 force-pushed the dvz/filter-by-published branch from 9e38163 to 6d3db8d Compare December 14, 2024 23:29

bradenmacdonald mentioned this pull request Dec 16, 2024

Allow filtering library by publish status [FC-0062] #1406

Closed

pomegranited suggested changes Dec 17, 2024

View reviewed changes

This was referenced Dec 17, 2024

feat: add publish status to library meilisearch index openedx/edx-platform#36031

Open

feat: shareable URLs for library components and searches [FC-0076] #1575

Merged

fix: address PR comments and improve coverage

73ef429

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow filtering library by publish status #1570

feat: allow filtering library by publish status #1570

DanielVZ96 commented Dec 13, 2024

openedx-webhooks commented Dec 13, 2024 •

edited

Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Dec 14, 2024 •

edited

Loading

pomegranited left a comment

pomegranited Dec 17, 2024

pomegranited Dec 17, 2024

pomegranited Dec 17, 2024

pomegranited Dec 17, 2024

pomegranited Dec 17, 2024

DanielVZ96 Dec 20, 2024

DanielVZ96 Jan 17, 2025

pomegranited Dec 17, 2024

pomegranited Dec 17, 2024

jmakowski1123 commented Jan 21, 2025

sdaitzman commented Jan 23, 2025

		@@ -163,6 +171,7 @@ export const SearchContextProvider: React.FC<{
		searchKeywords,

feat: allow filtering library by publish status #1570

Are you sure you want to change the base?

feat: allow filtering library by publish status #1570

Conversation

DanielVZ96 commented Dec 13, 2024

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Dec 13, 2024 • edited Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Dec 14, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmakowski1123 commented Jan 21, 2025

sdaitzman commented Jan 23, 2025

openedx-webhooks commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading