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

feat: removing public catalog's dependency on enterprise catalog default results #320

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

alex-sheehan-edx
Copy link
Contributor

Functionally, nothing changes with this PR as far as UX is concerned

image

What this PR does:

  • removes the CatalogNoResultsDeck component's call to the enterprise catalog service to fetch default algolia results.
  • instead initializes a new instant search component and makes the call directly

@alex-sheehan-edx alex-sheehan-edx force-pushed the asheehan-edx/removing-catalog-dependencies branch 10 times, most recently from 96a62f3 to ac98ec9 Compare June 20, 2023 17:10
@@ -90,18 +80,6 @@ describe('catalog no results deck works as expected', () => {
);
});
});
test('API error responses will hide content deck', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not relevant any more because there are no api errors to deal with!

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.75 ⚠️

Comparison is base (c26f42c) 83.24% compared to head (92abf79) 82.49%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
- Coverage   83.24%   82.49%   -0.75%     
==========================================
  Files          38       38              
  Lines         716      697      -19     
  Branches      221      219       -2     
==========================================
- Hits          596      575      -21     
- Misses        116      118       +2     
  Partials        4        4              
Impacted Files Coverage Δ
src/data/services/EnterpriseCatalogAPIService.js 100.00% <ø> (ø)
...ents/catalogNoResultsDeck/CatalogNoResultsDeck.jsx 73.07% <100.00%> (-16.12%) ⬇️
...ents/catalogSearchResults/CatalogSearchResults.jsx 85.33% <100.00%> (+0.09%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alex-sheehan-edx alex-sheehan-edx requested a review from a team June 21, 2023 14:23
@@ -523,23 +530,8 @@ describe('Main Catalogs view works as expected', () => {
),
).toBeInTheDocument();
});
test('no program search results displays popular programs text', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being taken out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're not testing the no results component directly, we're testing the mocked algolia component is rendered in when there are no hits. As such, we don't need two tests saying "does it say programs" and "does it say courses" any more, just "does it say".

@alex-sheehan-edx alex-sheehan-edx force-pushed the asheehan-edx/removing-catalog-dependencies branch from ac98ec9 to 718faf9 Compare June 21, 2023 16:02
@alex-sheehan-edx alex-sheehan-edx force-pushed the asheehan-edx/removing-catalog-dependencies branch 3 times, most recently from 666c578 to 088627e Compare June 21, 2023 18:32
@alex-sheehan-edx alex-sheehan-edx force-pushed the asheehan-edx/removing-catalog-dependencies branch from 088627e to 92abf79 Compare June 21, 2023 18:41
@alex-sheehan-edx alex-sheehan-edx merged commit 7ede119 into main Jun 21, 2023
4 of 5 checks passed
@alex-sheehan-edx alex-sheehan-edx deleted the asheehan-edx/removing-catalog-dependencies branch June 21, 2023 18:54
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.

2 participants