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: update save button state for certificate info #957

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

uzairr
Copy link
Contributor

@uzairr uzairr commented Mar 27, 2024

Update the save button state in case of editing the certificate heading or description.

PROD-3419

@@ -224,6 +230,8 @@ describe('courseInfo edit course actions', () => {
expect(changeMock).toHaveBeenNthCalledWith(8, 'in_year_value.per_lead_international', 20);
expect(changeMock).toHaveBeenNthCalledWith(9, 'in_year_value.per_click_usa', 25);
expect(changeMock).toHaveBeenNthCalledWith(10, 'in_year_value.per_click_international', 30);
expect(changeMock).toHaveBeenNthCalledWith(10, 'additional_metadata.certificate_info.heading', 'Test Certificate');
expect(changeMock).toHaveBeenNthCalledWith(10, 'additional_metadata.certificate_info.blurb', 'test blurb');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(changeMock).toHaveBeenNthCalledWith(10, 'additional_metadata.certificate_info.blurb', 'test blurb');
expect(changeMock).toHaveBeenNthCalledWith(12, 'additional_metadata.certificate_info.heading', 'Test Certificate');

update the Nth Called value for the rest of them

@@ -224,6 +230,8 @@ describe('courseInfo edit course actions', () => {
expect(changeMock).toHaveBeenNthCalledWith(8, 'in_year_value.per_lead_international', 20);
expect(changeMock).toHaveBeenNthCalledWith(9, 'in_year_value.per_click_usa', 25);
expect(changeMock).toHaveBeenNthCalledWith(10, 'in_year_value.per_click_international', 30);
expect(changeMock).toHaveBeenNthCalledWith(10, 'additional_metadata.certificate_info.heading', 'Test Certificate');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(changeMock).toHaveBeenNthCalledWith(10, 'additional_metadata.certificate_info.heading', 'Test Certificate');
expect(changeMock).toHaveBeenNthCalledWith(11, 'additional_metadata.certificate_info.heading', 'Test Certificate');

@@ -118,6 +118,12 @@ function updateFormValuesAfterSave(change, currentFormValues, initialValues) {
per_click_usa: perClickUSA,
per_click_international: perClicknternational,
},
additional_metadata: {
certificate_info: {
heading,
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to fix indentation issues

@uzairr uzairr force-pushed the update-save-state-for-cert-info branch 2 times, most recently from 72784f6 to 31b34c1 Compare April 1, 2024 09:54
@DawoudSheraz
Copy link
Contributor

Please fix failing tests.

@uzairr uzairr force-pushed the update-save-state-for-cert-info branch from 31b34c1 to 3270a95 Compare April 1, 2024 22:45
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.27%. Comparing base (dd579f2) to head (6641628).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
+ Coverage   67.25%   67.27%   +0.02%     
==========================================
  Files         128      128              
  Lines        3219     3221       +2     
  Branches      931      931              
==========================================
+ Hits         2165     2167       +2     
  Misses       1005     1005              
  Partials       49       49              

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

Comment on lines +70 to +78
flip={false}
icon={
Object {
"icon": Array [
512,
448,
512,
Array [],
"f0c5",
"M502.6 70.63l-61.25-61.25C435.4 3.371 427.2 0 418.7 0H255.1c-35.35 0-64 28.66-64 64l.0195 256C192 355.4 220.7 384 256 384h192c35.2 0 64-28.8 64-64V93.25C512 84.77 508.6 76.63 502.6 70.63zM464 320c0 8.836-7.164 16-16 16H255.1c-8.838 0-16-7.164-16-16L239.1 64.13c0-8.836 7.164-16 16-16h128L384 96c0 17.67 14.33 32 32 32h47.1V320zM272 448c0 8.836-7.164 16-16 16H63.1c-8.838 0-16-7.164-16-16L47.98 192.1c0-8.836 7.164-16 16-16H160V128H63.99c-35.35 0-64 28.65-64 64l.0098 256C.002 483.3 28.66 512 64 512h192c35.2 0 64-28.8 64-64v-32h-47.1L272 448z",
"M384 336H192c-8.8 0-16-7.2-16-16V64c0-8.8 7.2-16 16-16l140.1 0L400 115.9V320c0 8.8-7.2 16-16 16zM192 384H384c35.3 0 64-28.7 64-64V115.9c0-12.7-5.1-24.9-14.1-33.9L366.1 14.1c-9-9-21.2-14.1-33.9-14.1H192c-35.3 0-64 28.7-64 64V320c0 35.3 28.7 64 64 64zM64 128c-35.3 0-64 28.7-64 64V448c0 35.3 28.7 64 64 64H256c35.3 0 64-28.7 64-64V416H272v32c0 8.8-7.2 16-16 16H64c-8.8 0-16-7.2-16-16V192c0-8.8 7.2-16 16-16H96V128H64z",
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 snapshot update needed? In a previous change #869, this was not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this is because of the icon dependency on external library, it is recently updated by renovate,
https://github.com/openedx/frontend-app-publisher/blob/master/package-lock.json#L18

@uzairr uzairr force-pushed the update-save-state-for-cert-info branch from 3270a95 to 6641628 Compare April 2, 2024 11:53
@uzairr uzairr merged commit ad9b6a3 into master Apr 3, 2024
7 checks passed
@uzairr uzairr deleted the update-save-state-for-cert-info branch April 3, 2024 09: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