Fix #19792: raise error when tab RTE components are empty and prevent saving #20241

alice21mota · 2024-04-30T09:21:14Z

Overview

This PR fixes [BUG]: Unexpected error on using tab RTE component or collapsible RTE component empty #19792 .
This PR does the following: prevents user from clicking "Done" when at least one of the tab components is empty. Displays an error message indicating the error.
(For bug-fixing PRs only) The original bug occurred because: There was no validation for the tab RTE components, which would generate errors when saving empty components.

Essential Checklist

Please follow the instructions for making a code change.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
I have written tests for my code.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

316467319-bb25deb9-9a37-425e-840c-7706a3a15229.mov

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

Prevents user from saving when a component of a tab is empty, displaying an error message

oppiabot · 2024-04-30T09:21:26Z

Assigning @Lawful2002 for the first pass review of this PR. Thanks!

alice21mota · 2024-04-30T09:22:09Z

@seanlip PTAL

seanlip

Thanks @alice21mota! Left some notes.

core/templates/services/rte-helper-modal.controller.ts

oppiabot · 2024-05-03T10:11:32Z

Unassigning @seanlip since the review is done.

oppiabot · 2024-05-03T10:11:35Z

Hi @alice21mota, it looks like some changes were requested on this pull request by @seanlip. PTAL. Thanks!

dquote> change comment related to Value[0] in line 257 to be more specific dquote> replace tab with tabIndex dquote> make error messages more explicit for users dquote> fix tests to accomodate changes

change comment related to Value[0] in line 257 to be more specific replace tab with tabIndex make error messages more explicit for users fix tests to accomodate changes

alice21mota · 2024-05-03T22:21:10Z

Hi @seanlip PTAL!
I made the changes you requested, let me know if there are any remaining issues.
This is how it would appear now in case of empty component:

Screen.Recording.2024-05-03.at.19.02.08.mov

oppiabot · 2024-05-03T22:24:14Z

Unassigning @alice21mota since a re-review was requested. @alice21mota, please make sure you have addressed all review comments. Thanks!

alice21mota · 2024-05-08T13:46:59Z

Hi @seanlip!
I saw that PRs become stale after 7 days, and am a little worry the PR gets closed. Do you think my changes look good?
Are there any more changes you would like me to make?
Thank you for your attention :)

seanlip

Thanks @alice21mota. I think this looks good, just a few minor notes and it should be good to merge. Sorry for the delay, the past few days have been quite hectic.

Also, just a note -- please don't resolve review comments, let the reviewer do that. Otherwise the reviewer just has to open everything up again to check that the comments are addressed. Thanks!

seanlip · 2024-05-08T14:48:00Z

core/templates/services/rte-helper-modal.controller.spec.ts

@@ -356,21 +356,22 @@ describe('RteHelperModalComponent', () => {
      );
      expect(component.isErrorMessageNonempty()).toBe(true);
      expect(component.errorMessage).toBe(
-        'Please ensure that each title and content are filled'
+        'Please ensure that tab 1 title is filled.'


Please ensure that the title of tab 1 is filled

seanlip · 2024-05-08T14:48:09Z

core/templates/services/rte-helper-modal.controller.spec.ts

      component.customizationArgsForm.value[0][1].content = '';
      component.onCustomizationArgsFormChange(
        component.customizationArgsForm.value
      );
      expect(component.isErrorMessageNonempty()).toBe(true);
      expect(component.errorMessage).toBe(
-        'Please ensure that each title and content are filled'
+        'Please ensure that tab 2 content is filled.'


Please ensure that the content of tab 2 is filled

seanlip · 2024-05-08T14:48:35Z

core/templates/services/rte-helper-modal.controller.ts

-      // Value[0] corresponds to tab Contents.
-      for (let tab = 0; tab < value[0].length; tab++) {
-        if (value[0][tab].title === '' || value[0][tab].content === '') {
+      // Value[0] corresponds to all tabs Contents and titles.


all tab contents and titles.

(tabs --> tab; do not capitalize "Contents")

alice21mota · 2024-05-08T14:52:55Z

Thank you for the notes @seanlip ! Will address and commit them shortly.

alice21mota · 2024-05-08T17:14:24Z

Hi @seanlip ! Is it possible to re-run the checks? Many of them were cancelled after one failed, but I don´t believe it has anything to do with the changes I made. If not how should I proceed?

seanlip · 2024-05-08T17:18:23Z

@alice21mota Yup, this is a known flake (#19732). I'll re-run, thanks!

In future, if you see an error, please search for it on GitHub and see if it's filed already. If so then reference it when asking for a re-run; if not, then either the issue is with your PR or a new issue should be filed. Thanks!

alice21mota · 2024-05-08T17:34:00Z

Thank you @seanlip ! I believe the same thing occurred, not sure why. Will add the info to the existing issue.

seanlip · 2024-05-08T18:01:45Z

@alice21mota The new error is different, please follow the procedure I outlined above, thanks.

alter displayed messages when a tab component is not filled change corresponding tests accordingly fix comment typos

alice21mota · 2024-05-10T06:05:08Z

Hi @seanlip PTAL!
I made the requested changes. Hope it is all good now. Awaiting your feedback and thanks in advance.
In relation to one of the failed e2e tests, I believe it is related to the issue #17279, and therefore a flake. As for the other failed e2e test, e believe it may be due to some network connection error, but correct me if I am wrong.

oppiabot · 2024-05-10T06:08:12Z

Unassigning @alice21mota since a re-review was requested. @alice21mota, please make sure you have addressed all review comments. Thanks!

seanlip

It looks good, thanks @alice21mota. Let's try to rerun this once #20287 is merged.

One note, can you please respond to the existing review comments (with "Done" if you've done them)? Let's follow the proper review process. Thanks!

alice21mota · 2024-05-10T15:05:53Z

Done! Thank you @seanlip !
Can you give some guidance on the next steps? Thank you

seanlip · 2024-05-10T16:45:31Z

@alice21mota I've merged develop into your branch. Let's see if it passes, if so we can merge this once @Lawful2002 reviews.

@Lawful2002 could you please review for codeowners? Thanks.

@alice21mota feel free to take up another issue in the meantime, if you like!

alice21mota · 2024-05-10T16:53:05Z

Thank you @seanlip ! Will do :)

oppiabot · 2024-05-14T16:17:53Z

Unassigning @Lawful2002 since they have already approved the PR.

oppiabot · 2024-05-14T16:17:57Z

Hi @alice21mota, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

alice21mota and others added 2 commits April 30, 2024 09:35

Fix: Handle empty content error in tab interaction

4d03853

Prevents user from saving when a component of a tab is empty, displaying an error message

fix: linting errors and unnecessary code removed

93b6197

alice21mota requested a review from a team as a code owner April 30, 2024 09:21

alice21mota requested a review from Lawful2002 April 30, 2024 09:21

oppiabot bot assigned Lawful2002 Apr 30, 2024

oppiabot bot assigned seanlip Apr 30, 2024

seanlip requested changes May 3, 2024

View reviewed changes

core/templates/services/rte-helper-modal.controller.ts Outdated Show resolved Hide resolved

core/templates/services/rte-helper-modal.controller.ts Outdated Show resolved Hide resolved

core/templates/services/rte-helper-modal.controller.ts Outdated Show resolved Hide resolved

oppiabot bot unassigned seanlip May 3, 2024

oppiabot bot assigned alice21mota May 3, 2024

alice21mota added 3 commits May 3, 2024 21:30

fix: changes request by reviwer

e8343d6

dquote> change comment related to Value[0] in line 257 to be more specific dquote> replace tab with tabIndex dquote> make error messages more explicit for users dquote> fix tests to accomodate changes

fix: changes request by reviwer

6101701

dquote> change comment related to Value[0] in line 257 to be more specific dquote> replace tab with tabIndex dquote> make error messages more explicit for users dquote> fix tests to accomodate changes

fix: changes request by reviwer

4a6fe73

change comment related to Value[0] in line 257 to be more specific replace tab with tabIndex make error messages more explicit for users fix tests to accomodate changes

oppiabot bot assigned seanlip and unassigned alice21mota May 3, 2024

alice21mota requested a review from seanlip May 6, 2024 20:08

seanlip requested changes May 8, 2024

View reviewed changes

seanlip assigned alice21mota and unassigned seanlip May 8, 2024

alice21mota force-pushed the validation-tabs-rte-component branch 2 times, most recently from 5514e3d to 7550dd5 Compare May 8, 2024 15:32

alice21mota force-pushed the validation-tabs-rte-component branch 2 times, most recently from 7550dd5 to 4a6fe73 Compare May 9, 2024 09:09

fix: displayed errors messages

583bd1c

alter displayed messages when a tab component is not filled change corresponding tests accordingly fix comment typos

oppiabot bot assigned seanlip and unassigned alice21mota May 10, 2024

seanlip approved these changes May 10, 2024

View reviewed changes

seanlip assigned alice21mota and unassigned seanlip May 10, 2024

seanlip added 2 commits May 11, 2024 00:28

Merge branch 'develop' into validation-tabs-rte-component

0c88c05

Merge branch 'develop' into validation-tabs-rte-component

599b65e

seanlip enabled auto-merge May 10, 2024 16:45

Lawful2002 approved these changes May 14, 2024

View reviewed changes

seanlip added this pull request to the merge queue May 14, 2024

oppiabot bot unassigned Lawful2002 May 14, 2024

oppiabot bot added the PR: LGTM label May 14, 2024

Merged via the queue into oppia:develop with commit 59f92d5 May 14, 2024
81 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #19792: raise error when tab RTE components are empty and prevent saving #20241

Fix #19792: raise error when tab RTE components are empty and prevent saving #20241

alice21mota commented Apr 30, 2024 •

edited

oppiabot bot commented Apr 30, 2024

alice21mota commented Apr 30, 2024

seanlip left a comment

oppiabot bot commented May 3, 2024

oppiabot bot commented May 3, 2024

alice21mota commented May 3, 2024

oppiabot bot commented May 3, 2024

alice21mota commented May 8, 2024

seanlip left a comment

seanlip May 8, 2024

alice21mota May 10, 2024

seanlip May 8, 2024

alice21mota May 10, 2024

seanlip May 8, 2024

alice21mota May 10, 2024

alice21mota commented May 8, 2024

alice21mota commented May 8, 2024

seanlip commented May 8, 2024

alice21mota commented May 8, 2024

seanlip commented May 8, 2024

alice21mota commented May 10, 2024

oppiabot bot commented May 10, 2024

seanlip left a comment

alice21mota commented May 10, 2024 •

edited

seanlip commented May 10, 2024 •

edited

alice21mota commented May 10, 2024

oppiabot bot commented May 14, 2024

oppiabot bot commented May 14, 2024

Fix #19792: raise error when tab RTE components are empty and prevent saving #20241

Fix #19792: raise error when tab RTE components are empty and prevent saving #20241

Conversation

alice21mota commented Apr 30, 2024 • edited

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Apr 30, 2024

alice21mota commented Apr 30, 2024

seanlip left a comment

Choose a reason for hiding this comment

oppiabot bot commented May 3, 2024

oppiabot bot commented May 3, 2024

alice21mota commented May 3, 2024

oppiabot bot commented May 3, 2024

alice21mota commented May 8, 2024

seanlip left a comment

Choose a reason for hiding this comment

seanlip May 8, 2024

Choose a reason for hiding this comment

alice21mota May 10, 2024

Choose a reason for hiding this comment

seanlip May 8, 2024

Choose a reason for hiding this comment

alice21mota May 10, 2024

Choose a reason for hiding this comment

seanlip May 8, 2024

Choose a reason for hiding this comment

alice21mota May 10, 2024

Choose a reason for hiding this comment

alice21mota commented May 8, 2024

alice21mota commented May 8, 2024

seanlip commented May 8, 2024

alice21mota commented May 8, 2024

seanlip commented May 8, 2024

alice21mota commented May 10, 2024

oppiabot bot commented May 10, 2024

seanlip left a comment

Choose a reason for hiding this comment

alice21mota commented May 10, 2024 • edited

seanlip commented May 10, 2024 • edited

alice21mota commented May 10, 2024

oppiabot bot commented May 14, 2024

oppiabot bot commented May 14, 2024

alice21mota commented Apr 30, 2024 •

edited

alice21mota commented May 10, 2024 •

edited

seanlip commented May 10, 2024 •

edited