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 94.54545% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.24%. Comparing base (31f39cb) to head (a338fe8).
Report is 1 commits behind head on master.

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1570   +/-   ##
=======================================
  Coverage   93.24%   93.24%           
=======================================
  Files        1100     1101    +1     
  Lines       21803    21859   +56     
  Branches     4632     4644   +12     
=======================================
+ Hits        20330    20383   +53     
- Misses       1408     1411    +3     
  Partials       65       65

☔ 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 · 2025-02-03T22:45:48Z

@pomegranited @jmakowski1123 @sdaitzman

I made it so that the two filters that display content are auto selected when ordering by recently published, and also disabled the other filter since it wouldn't show anything even when selected. I also added the badges that show the selected filters.

pomegranited · 2025-02-03T23:34:43Z

src/search-manager/SearchManager.ts

@@ -99,6 +106,7 @@ export const SearchContextProvider: React.FC<{
  const [searchKeywords, setSearchKeywords] = React.useState('');
  const [blockTypesFilter, setBlockTypesFilter] = React.useState<string[]>([]);
  const [problemTypesFilter, setProblemTypesFilter] = React.useState<string[]>([]);
+  const [publishStatusFilter, setPublishStatusFilter] = React.useState<PublishStatus[]>([]);


When you resolve the conflicts for this file, please note that all of the search state variables are now managed as URL parameters, so this status should be too. Should look something like this:

const [publishStatusFilter, setPublishStatusFilter] = useStateOrUrlSearchParam<PublishStatus>( [], 'published', (value: string) => Object.values(PublishStatus).find((enumValue) => value === enumValue), (value: PublishStatus) => value.toString(), skipUrlUpdate, );

References:

searchSortOrder uses an enum of allowed values

tagsFilter allows multiple values to be selected.

…shed

pomegranited

Hi @DanielVZ96 , this looks great :) But I found a couple of bugs, could you take another look?

pomegranited · 2025-02-04T02:22:33Z

src/library-authoring/components/ComponentCard.tsx

@@ -228,6 +230,7 @@ const ComponentCard = ({ contentHit }: ComponentCardProps) => {
          )}
        </ActionRow>
      )}
+      hasUnpublishedChanges={modified >= (lastPublished ?? 0)}


nit: Why not pull this from the added published_status field? I'm ok either way, but it seems like we shouldn't duplicate functionality here.

ohh nice catch

pomegranited · 2025-02-04T02:31:05Z

src/library-authoring/LibraryAuthoringPage.tsx

@@ -266,6 +267,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage
              <SearchKeywordsField className="mr-3" />
              <FilterByTags />
              {!insideCollections && <FilterByBlockType />}
+              <FilterByPublished />


Could you please add <FilterByPublished /> to the LibraryCollectionPage too?

pomegranited · 2025-02-04T02:58:36Z

src/search-manager/FilterByPublished.tsx

+    }
+  }, [searchSortOrder]);
+
+  const toggleFilterMode = React.useCallback((mode: PublishStatus) => {


Bug: Selecting one publish_status filter clears all the others? Maybe see FilterByBlockType for how its checkbox onChange handler works.

it was because I forgot to add the setter to the dependencies. this was somehow breaking the tags filter tests so thank you so much for finding this :)

Thank you for working out a simple fix! it's working great now.

pomegranited

👍 This looks and works great @DanielVZ96 , thank you for accommodating my suggestions and the UI tweaks from Product!

I tested this using the PR test instructions, plus tested using these filters on the Collection page.
I read through the code
I checked for accessibility issues by using my keyboard to navigate
~~Includes documentation~~ N/A
User-facing strings are extracted for translation

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

Merged

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

Merged

fix: address PR comments and improve coverage

73ef429

feat: update published filter list and update filters when sorted

3f0f95e

DanielVZ96 requested a review from pomegranited February 3, 2025 22:45

pomegranited reviewed Feb 3, 2025

View reviewed changes

DanielVZ96 added 8 commits February 3, 2025 20:48

Merge remote-tracking branch 'origin/master' into dvz/filter-by-publi…

2effa32

…shed

fix: use url for publish status

09d1284

fix: remove unused code

54913f7

fix: setProblemTypesFilter usage

777fe25

fix: openinfosidebar removal update

f28faaa

fix: duped filters

fd35ce8

fix: extract label to message

419ac2f

fix: missing import

dc3f657

pomegranited suggested changes Feb 4, 2025

View reviewed changes

fix: publish filter dependencies

daa98b8

DanielVZ96 added 2 commits February 4, 2025 12:33

fix: use publish status for component card label

19f9420

fix: tests

a338fe8

DanielVZ96 requested a review from pomegranited February 4, 2025 16:02

pomegranited approved these changes Feb 5, 2025

View reviewed changes

ChrisChV approved these changes Feb 5, 2025

View reviewed changes

ChrisChV merged commit 05dddce into openedx:master Feb 5, 2025
7 checks passed

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

DanielVZ96 commented Feb 3, 2025

pomegranited Feb 3, 2025

pomegranited left a comment

pomegranited Feb 4, 2025

DanielVZ96 Feb 4, 2025

pomegranited Feb 4, 2025

DanielVZ96 Feb 4, 2025

pomegranited Feb 4, 2025

DanielVZ96 Feb 4, 2025

pomegranited Feb 5, 2025

pomegranited left a comment

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

feat: allow filtering library by publish status #1570

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

DanielVZ96 commented Feb 3, 2025

Choose a reason for hiding this comment

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

pomegranited left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading