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

fix: remove broken usage of useCache from getSubsidyRequestConfiguration #1030

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

iloveagent57
Copy link
Contributor

@iloveagent57 iloveagent57 commented Sep 14, 2023

Fixes an issue where updates to course requests settings weren't reflected in the Settings/Access page. We were previously setting useCache to always be true in the call to fetch subsidy request configs.

Broken behavior was possibly related to this change? openedx/frontend-platform#361

Only if submitting a visual change

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

return EnterpriseAccessApiService.apiClient({
useCache: configuration.USE_API_CACHE,
}).get(url, { clearCacheEntry });
return EnterpriseAccessApiService.apiClient().get(url, { clearCacheEntry });
Copy link
Member

Choose a reason for hiding this comment

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

[curious] If this API call never uses useCache anymore, is the clearCacheEntry logic necessary anymore in this function's signature, passed to this get function, or in the caller(s) of getSubsidyRequestConfiguration?

@adamstankiewicz
Copy link
Member

Broken behavior was possibly related to this change? openedx/frontend-platform#361

Hmm, yeah seems likely especially around the mention of "Usages of clearCacheEntry will have to be updated."... probably something that we missed 🙃

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage: 92.96% and project coverage change: +0.11% 🎉

Comparison is base (31f4dc8) 82.89% compared to head (f0f8189) 83.01%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1030      +/-   ##
==========================================
+ Coverage   82.89%   83.01%   +0.11%     
==========================================
  Files         402      412      +10     
  Lines        8735     8849     +114     
  Branches     1805     1820      +15     
==========================================
+ Hits         7241     7346     +105     
- Misses       1455     1464       +9     
  Partials       39       39              
Files Changed Coverage Δ
.../components/ContactCustomerSupportButton/index.jsx 100.00% <ø> (ø)
src/components/settings/SettingsLMSTab/index.jsx 95.45% <ø> (ø)
src/data/reducers/portalConfiguration.js 73.33% <ø> (ø)
src/data/services/LmsApiService.js 33.92% <12.50%> (-1.08%) ⬇️
src/components/settings/SettingsTabs.jsx 87.87% <80.00%> (-2.45%) ⬇️
.../settings/SettingsApiCredentialsTab/CopyButton.jsx 92.85% <92.85%> (ø)
src/components/settings/HelpCenterButton.jsx 100.00% <100.00%> (ø)
...s/SettingsApiCredentialsTab/APICredentialsPage.jsx 100.00% <100.00%> (ø)
...nts/settings/SettingsApiCredentialsTab/Context.jsx 100.00% <100.00%> (ø)
...settings/SettingsApiCredentialsTab/CopiedToast.jsx 100.00% <100.00%> (ø)
... and 9 more

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

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@iloveagent57 iloveagent57 merged commit 22f7c2c into master Sep 14, 2023
5 checks passed
@iloveagent57 iloveagent57 deleted the aed/ent-7696 branch September 14, 2023 20:46
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