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: Adding search/filtering for top-down assignment #1047

Merged
merged 14 commits into from
Oct 11, 2023
Merged

Conversation

kiram15
Copy link
Contributor

@kiram15 kiram15 commented Oct 2, 2023

Screenshot 2023-10-02 at 3 17 19 PM

Jira ticket

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@kiram15 kiram15 requested review from adamstankiewicz and a team October 2, 2023 21:21
return cIndex;
}, [config.ALGOLIA_INDEX_NAME, searchClient]);

const searchFilters = `enterprise_catalog_query_uuids:${offerId}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to confirm that this is the correct filter, only allowing admins to see what is associated with the offerId fetched from the data analytics api.

Copy link
Member

@adamstankiewicz adamstankiewicz Oct 5, 2023

Choose a reason for hiding this comment

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

@kiram15 The Algolia filter should be based on a catalog uuid, not a catalog query uuid. The admin portal frontend is generally unaware of catalog queries, only catalog uuids.

As previously discussed, we unfortunately don't have access to the catalog UUID for this page currently as the analytics API (like you noted earlier) only returns catalog_name, not catalog_uuid. We will be updating the data source for the LCM feature to no longer pull from the analytics API, in favor of pulling directly from enterprise-access/enterprise-subsidy. Until then, though, we won't have access to a catalog uuid to use in the search filter.

For the short term, we could either temporarily hardcode a catalog UUID until the data is available to the UI (behind a feature flag so not really a concern other than not being able to adequately test your changes on stage/prod at the moment) OR (probably preferable) perhaps tap into the enterprise_customer_catalogs returned by the https://courses.edx.org/enterprise/api/v1/enterprise-customer/dashboard_list/ API (the source of the enterprise customer metadata for admin portal).

Happy to sync up about short-term options in Slack!

src/components/learner-credit-management/BudgetCard-V2.jsx Outdated Show resolved Hide resolved
src/data/hooks.js Outdated Show resolved Hide resolved
src/components/learner-credit-management/utils.jsx Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

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

Comparison is base (deaedad) 83.24% compared to head (a3c1812) 83.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1047      +/-   ##
==========================================
- Coverage   83.24%   83.20%   -0.05%     
==========================================
  Files         428      432       +4     
  Lines        9098     9164      +66     
  Branches     1861     1875      +14     
==========================================
+ Hits         7574     7625      +51     
- Misses       1482     1497      +15     
  Partials       42       42              
Files Coverage Δ
src/components/ContentHighlights/data/constants.js 100.00% <ø> (ø)
...edit-management/BudgetDetailCatalogTabContents.jsx 100.00% <100.00%> (ø)
...onents/learner-credit-management/data/constants.js 100.00% <100.00%> (ø)
...components/learner-credit-management/data/utils.js 95.16% <100.00%> (+0.16%) ⬆️
...learner-credit-management/search/CatalogSearch.jsx 100.00% <100.00%> (ø)
src/components/test/testUtils.jsx 75.00% <ø> (ø)
...-credit-management/search/CatalogSearchResults.jsx 95.65% <95.65%> (ø)
...nts/learner-credit-management/cards/CourseCard.jsx 80.00% <80.00%> (ø)
src/data/hooks.js 35.71% <20.00%> (-3.42%) ⬇️
...ents/learner-credit-management/data/hooks/hooks.js 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

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

src/data/hooks.js Outdated Show resolved Hide resolved
src/components/learner-credit-management/BudgetCard-V2.jsx Outdated Show resolved Hide resolved
src/components/learner-credit-management/BudgetCard-V2.jsx Outdated Show resolved Hide resolved
src/components/learner-credit-management/algoliaUtils.js Outdated Show resolved Hide resolved
src/components/learner-credit-management/algoliaUtils.js Outdated Show resolved Hide resolved
src/components/learner-credit-management/utils.jsx Outdated Show resolved Hide resolved
src/data/constants/learnerCredit.js Outdated Show resolved Hide resolved
src/data/constants/learnerCredit.js Outdated Show resolved Hide resolved
src/data/hooks.js Show resolved Hide resolved
src/data/hooks.js Outdated Show resolved Hide resolved
@kiram15 kiram15 merged commit f902ee1 into master Oct 11, 2023
3 of 5 checks passed
@kiram15 kiram15 deleted the kiram15/ENT-7339 branch October 11, 2023 21:28
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.

3 participants