feat: add content search modal [FC-0040] #928

rpenido · 2024-03-29T01:19:46Z

Description

This PR implements a front-end UI to the metasearch engine prototype. The modal is shown after clicking the new search icon button in the Studio Header.

The feature is only enabled if the MEILISEARCH_ENABLED env variable is set to true.

More Info

Closes [Course Search] Trigger Course Search Modal in Course Authoring MFE modular-learning#199
Depends on feat: add search menu button to header [FC-0040] frontend-component-header#474

Testing Instructions

Run your local stack on this branch
Make sure you have meilisearch setup locally, follow the setup instructions here https://github.com/open-craft/tutor-contrib-meilisearch, but use the following branch for the plugin (https://github.com/rpenido/tutor-contrib-meilisearch/tree/rpenido/fal-3693-pass-meilisearch-enabled-env-to-mfe-config) to make sure the MEILISEARCH_ENABLED is configured in the MFEs.
Make sure you also have some sample taxonomies/tags. You can add some from here: https://github.com/open-craft/taxonomy-sample-data
Run tutor dev run cms bash and ./manage.py cms reindex_studio
Click on the search icon in the Studio Header
Search and refine.

NOTE: If you don't see the search icon in the header, check the following URL: http://local.edly.io:8000/api/mfe_config/v1

You must have the MEILISEARCH_ENABLED config as show below:

If you don't have this configuration, check step 2.

Private ref: FAL-3693

openedx-webhooks · 2024-03-29T01:19:50Z

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:

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.

codecov · 2024-03-29T01:31:42Z

Codecov Report

Attention: Patch coverage is 96.15385% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.01%. Comparing base (7f3164b) to head (865c334).

Files	Patch %	Lines
src/header/Header.jsx	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
+ Coverage   92.00%   92.01%   +0.01%     
==========================================
  Files         612      616       +4     
  Lines       10745    10771      +26     
  Branches     2304     2312       +8     
==========================================
+ Hits         9886     9911      +25     
- Misses        830      831       +1     
  Partials       29       29

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

rpenido · 2024-03-29T03:52:18Z

package.json

@@ -43,7 +43,7 @@
    "@edx/brand": "npm:@openedx/brand-openedx@^1.2.2",
    "@edx/frontend-component-ai-translations": "^2.0.0",
    "@edx/frontend-component-footer": "^13.0.2",
-    "@edx/frontend-component-header": "^5.0.2",
+    "@edx/frontend-component-header": "github:open-craft/frontend-component-header#rpenido/dist/fal-3693-add-search-menu-button",


Update after merge:

feat: add search menu button to header [FC-0040] frontend-component-header#474

Done: 6f05189

yusuf-musleh

👍 Great work on this @rpenido ! Everything looks good to me, just left a few comments on the test cases.

I tested this: I following the testing instruction in the PR, it works as expected
I read through the code
I checked for accessibility issues
~~Includes documentation~~

yusuf-musleh · 2024-03-31T10:22:22Z

src/studio-home/StudioHome.test.jsx

+    it('should render Studio home title', () => {
+      render(<RootWrapper />);
+      screen.logTestingPlaygroundURL();
+    });


Is this testcase missing a check or testing something?

Hi @yusuf-musleh! Thank you for the review.
Cleaned here: 0d58997

yusuf-musleh · 2024-03-31T10:24:39Z

src/studio-home/StudioHome.test.jsx

+    it('should show search button and open search modal when button clicked, if search flag enabled', async () => {
+      setConfig({
+        ...getConfig(),
+        MEILISEARCH_ENABLED: 'false',
+      });
+      const { queryByRole } = render(<RootWrapper />);
+      expect(queryByRole('button', { name: 'Search content' })).not.toBeInTheDocument();
+    });
+
+    it('should show search button if meilisearch flag disabled', async () => {
+      setConfig({
+        ...getConfig(),
+        MEILISEARCH_ENABLED: 'false',
+      });
+      const { queryByRole } = render(<RootWrapper />);
+      expect(queryByRole('button', { name: 'Search content' })).not.toBeInTheDocument();
+    });


These 2 testcases seem to have the same test code? And it seems to be doing the opposite of what is described in the first case, and the second case should be "should not show" right?

I didn't check if the search button was showing in the header. This test was not working. I removed it here: 0d58997

Thank you for pointing out!

bradenmacdonald · 2024-04-03T19:49:37Z

@rpenido Now that the header changes have merged, I have pulled your changes here into #918 . Can you test that and let me know if it looks good? If you're happy with it, we can just close this PR. Or we can still merge it first, whichever is easier.

rpenido · 2024-04-03T22:16:43Z

@rpenido Now that the header changes have merged, I have pulled your changes here into #918 . Can you test that and let me know if it looks good? If you're happy with it, we can just close this PR. Or we can still merge it first, whichever is easier.

@bradenmacdonald Sure! I'll test it first thing tomorrow!

xitij2000

I understand most of the UI is placeholder. So I haven't looked at it too deeply.

Sorry for the delay, but I had some difficulty getting everything running. Running tutor dev launch again fixed the API key issue, and the indexing command you mentioned fixed the missing index issue.

This is good to go.

xitij2000 · 2024-04-02T08:50:03Z

webpack.dev-tutor.config.js

Please remove this file.

Done: beeaafa

Thank you for the review @xitij2000!

@xitij2000 Do you prefer to merge this or the @bradenmacdonald PR (#918)?

Does that include all the changes from this?

Even so I think it's better to have smaller changes. Could you please fix the merge issues and I'll merge .

@xitij2000 Yes, it does.

I updated it here with master changes and let npm install handle the conflicts in the package-lock.json file. I got a huge diff and some dependency package updates.

It seems the bot updates the package but not the dependencies..

Have I done something wrong?

I think when you run npm install on a broken package-lock, it regenerates it from scratch which might resolve some dependencies differently. I think it should be OK.

To resolve issues with package-lock.json I normally recommend something like this:

Checkout master and run npm ci so your package-lock.json matches master exactly.

Checkout your branch, and reset any changes to package-lock.json, so there are only changes to package.json showing in git.

Run npm install (inside the Tutor container, not on the host)

…icking-search-icon-in-header

openedx-webhooks · 2024-04-08T16:09:17Z

@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.

bradenmacdonald and others added 8 commits March 21, 2024 14:36

feat: Prototype search UI using Instantsearch + Meilisearch

80ef950

feat: Refine by XBlock type too

ed28225

chore: update to match backend changes

50ed3dd

feat: display breadcrumbs

130ab39

chore: convert back to JSX

8a963a9

chore: change modal dialog title to match design

f454ec8

feat: add search icon and tests

fc8759a

test: commenting tests

5b28ad2

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

rpenido added 4 commits March 29, 2024 00:21

fix: react key error at InfinitHits

9ce7eae

fix: meiliSearchEnabled config

32b5a90

test: fix tests

87c8032

refactor: remove unused code

ce521ea

rpenido commented Mar 29, 2024

View reviewed changes

refactor: remove unused message

493456c

rpenido force-pushed the rpenido/fal-3693-open-search-modal-when-clicking-search-icon-in-header branch from 2da3cc8 to 493456c Compare March 29, 2024 03:54

docs: adding comments and skiping some tests

5ee5934

rpenido mentioned this pull request Mar 29, 2024

feat: add search menu button to header [FC-0040] openedx/frontend-component-header#474

Merged

rpenido changed the title ~~feat: add content search modal~~ feat: add content search modal [FC-0040] Mar 29, 2024

refactor: moving api and adding more tests

7586280

rpenido requested a review from yusuf-musleh March 29, 2024 15:48

yusuf-musleh approved these changes Mar 31, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3693-open-search-modal-when-clicking-search-icon-in-header branch 4 times, most recently from 85f4c4f to a017f13 Compare March 31, 2024 23:51

test: removing unused tests

0d58997

rpenido force-pushed the rpenido/fal-3693-open-search-modal-when-clicking-search-icon-in-header branch from a017f13 to 0d58997 Compare April 1, 2024 00:02

bradenmacdonald mentioned this pull request Apr 1, 2024

Hacky Prototype search UI using Instantsearch + Meilisearch [FC-0040] #868

Closed

pomegranited mentioned this pull request Apr 4, 2024

Restrict search results based on user permissions [FC-0040] openedx/edx-platform#34471

Merged

xitij2000 approved these changes Apr 4, 2024

View reviewed changes

rpenido added 2 commits April 4, 2024 09:42

fix: removing tutor config file

beeaafa

chore: update edx/frontend-component-header dep

6f05189

rpenido marked this pull request as ready for review April 4, 2024 12:47

rpenido requested a review from a team as a code owner April 4, 2024 12:47

rpenido added 2 commits April 4, 2024 10:07

refactor: updating deprecated component

2be5059

test: fix typo

7fa6127

rpenido force-pushed the rpenido/fal-3693-open-search-modal-when-clicking-search-icon-in-header branch from fb25ce0 to 7fa6127 Compare April 5, 2024 14:37

Merge branch 'master' into rpenido/fal-3693-open-search-modal-when-cl…

865c334

…icking-search-icon-in-header

xitij2000 merged commit 2adff6e into openedx:master Apr 8, 2024
6 checks passed

xitij2000 deleted the rpenido/fal-3693-open-search-modal-when-clicking-search-icon-in-header branch April 8, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add content search modal [FC-0040] #928

feat: add content search modal [FC-0040] #928

rpenido commented Mar 29, 2024 •

edited

Loading

openedx-webhooks commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

rpenido Mar 29, 2024

rpenido Apr 4, 2024

yusuf-musleh left a comment

yusuf-musleh Mar 31, 2024

rpenido Apr 1, 2024

yusuf-musleh Mar 31, 2024

rpenido Apr 1, 2024

bradenmacdonald commented Apr 3, 2024

rpenido commented Apr 3, 2024 •

edited

Loading

xitij2000 left a comment

xitij2000 Apr 2, 2024

rpenido Apr 4, 2024

rpenido Apr 4, 2024

xitij2000 Apr 5, 2024

rpenido Apr 5, 2024 •

edited

Loading

xitij2000 Apr 8, 2024

bradenmacdonald Apr 8, 2024

openedx-webhooks commented Apr 8, 2024

feat: add content search modal [FC-0040] #928

feat: add content search modal [FC-0040] #928

Conversation

rpenido commented Mar 29, 2024 • edited Loading

Description

More Info

Testing Instructions

openedx-webhooks commented Mar 29, 2024 • edited Loading

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusuf-musleh 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

bradenmacdonald commented Apr 3, 2024

rpenido commented Apr 3, 2024 • edited Loading

xitij2000 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

rpenido Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Apr 8, 2024

rpenido commented Mar 29, 2024 •

edited

Loading

openedx-webhooks commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

rpenido commented Apr 3, 2024 •

edited

Loading

rpenido Apr 5, 2024 •

edited

Loading