Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Endpoint usage implementation for Courseware Search #1232

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

rijuma
Copy link
Member

@rijuma rijuma commented Oct 31, 2023

Description

Ticket: KBK-45 🔒

This change takes the the Search Results UI components 🔒 and connects it to the search endpoint. I'm taking some small liberties on the design and added some extra changes to the components due to some unplanned adaptations I had to make.

Important

These changes add he joi dependency to be used on payload validation.

UI changes

Showing results

courseware-search-screenshot

Added error alert in case of endpoint failure or wrong payload format.

search-error

Testing Instructions:

This feature is behind a Waffle Flag. In order to test it:

@rijuma rijuma changed the title Rijuma/courseware search integration Endpoint usage implementation for Courseware Search Oct 31, 2023
@rijuma rijuma changed the title Endpoint usage implementation for Courseware Search Endpoint usage implementation for Courseware Search (WIP) Oct 31, 2023
@rijuma rijuma force-pushed the rijuma/courseware-search-integration branch from 3db6ac0 to 0c37f9c Compare October 31, 2023 20:51
@rijuma rijuma force-pushed the rijuma/courseware-search-integration branch from 0c37f9c to cec3b00 Compare November 1, 2023 12:59
content: {
displayName,
htmlContent,
transcriptEn,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might find more of these types that have to be located like you do here and mapped like you do below, it might make sense to pull the content handling out to a separate controllable function

took, total, maxScore, accessDeniedCount,
} = data;

// TODO: Remove when publishing to prod. Just temporary for performance debugging.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nice, is there a way we can have it do this in prod if we set some local something in the browser to turn it on?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Maybe we can just leave this here? Or figure out a way to send this result to Newrelic or other monitoring tools?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due this is not sensitive information, we can leave this until find the best tool for tracking this, like Kibana or similars.

Copy link
Contributor

@schenedx schenedx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added my inline comments. Looking good so far.

src/course-home/courseware-search/CoursewareSearchForm.jsx Outdated Show resolved Hide resolved
took, total, maxScore, accessDeniedCount,
} = data;

// TODO: Remove when publishing to prod. Just temporary for performance debugging.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Maybe we can just leave this here? Or figure out a way to send this result to Newrelic or other monitoring tools?

@germanolleunlp germanolleunlp marked this pull request as ready for review November 8, 2023 14:47
Copy link
Contributor

@schenedx schenedx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (a64f0e0) 88.20% compared to head (988a047) 87.46%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
- Coverage   88.20%   87.46%   -0.74%     
==========================================
  Files         274      275       +1     
  Lines        4628     4723      +95     
  Branches     1165     1193      +28     
==========================================
+ Hits         4082     4131      +49     
- Misses        532      575      +43     
- Partials       14       17       +3     
Files Coverage Δ
...home/courseware-search/CoursewareResultsFilter.jsx 100.00% <100.00%> (ø)
...e-home/courseware-search/CoursewareSearchEmpty.jsx 100.00% <100.00%> (ø)
...se-home/courseware-search/CoursewareSearchForm.jsx 100.00% <ø> (ø)
src/course-home/courseware-search/messages.js 100.00% <ø> (ø)
src/course-tabs/CourseTabsNavigation.jsx 100.00% <100.00%> (ø)
...home/courseware-search/CoursewareSearchResults.jsx 90.47% <87.50%> (-9.53%) ⬇️
...urse-home/courseware-search/map-search-response.js 97.05% <97.05%> (ø)
src/course-home/data/api.js 86.95% <0.00%> (-3.05%) ⬇️
src/course-home/data/thunks.js 62.85% <0.00%> (-15.72%) ⬇️
...course-home/courseware-search/CoursewareSearch.jsx 44.68% <36.84%> (-26.75%) ⬇️

... and 1 file with indirect coverage changes

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

@germanolleunlp germanolleunlp merged commit a70a26f into master Nov 9, 2023
3 of 5 checks passed
@germanolleunlp germanolleunlp deleted the rijuma/courseware-search-integration branch November 9, 2023 16:14
@germanolleunlp germanolleunlp changed the title Endpoint usage implementation for Courseware Search (WIP) Endpoint usage implementation for Courseware Search Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants