[BUG]: Unexpected error on Exploration Editor page RTE component modal when start time is greater than end time

[shricodev]

Describe the bug

When creating an exploration on the Creator Dashboard page, if a video is added from RTE with a start time greater than the end time, the site reloads and displays an unexpected error of nonStrictValidationFailure

Error Trace:

nonStrictValidationError: [{"cmd":"edit_state_property","new_value":{"html":"<oppia-noninteractive-video _nghost-mdr-c48=\"\" autoplay-with-value=\"true\" end-with-value=\"10\" ng-version=\"11.2.14\" start-with-value=\"12\" video_id-with-value=\"&amp;quot;adJFT6_j9Uk&amp;quot;\"></oppia-noninteractive-video>","content_id":"content_0"},"old_value":{"html":"","content_id":"content_0"},"property_name":"content","state_name":"Introduction"}]

Steps To Reproduce

1. Setup the development server
2. Go to the Creator Dashboard page http://localhost:8181/creator-dashboard
3. Add an exploration with a video
4. Make sure to have a start time greater than the end time and click on Done.
5. Click on Save Content
6. This should trigger the error

Expected Behavior

When the user inputs a start time that is longer than the end time, as is done when the user adds a start or end time that is negative, the user shouldn't be allowed to select "Done."

Screenshots/Videos

start_greater_end.mp4

What device are you using?

Desktop

Operating System

Windows

What browsers are you seeing the problem on?

Edge

Browser version

No response

Additional context

Similar Issue: #18204

[yungmulah]

Hello, I have implemented a validation of the fields in rte-helper-modal.controller.ts and the message to be raised in rte-helper-modal.component.html so that users are not able to press Done if the start time is greater than the end time. Here is a video of how it looks with my implementation. Can I please get this issue assigned to me?

oppia_issue_film.mov

[yungmulah]

Hi @shricodev @kshitij01042002 do you think this solution looks good?

[kshitij01042002]

Hi @yungmulah thanks for the solution. The solution looks good to me.

@shricodev please let us know if it satisfies your expectations.

[shricodev]

@kshitij01042002 The change seems correct to me as well.
Just a quick check, @yungmulah, what if start_time equals end_time? Is it handled correctly already?

[yungmulah]

@shricodev I made it so that start time < end time or else the error will trigger since it does not make sense to me to have it set to the same, please correct me if I am wrong here. I can add an additional prompt if they are equal if that is better.

[shricodev]

@yungmulah Yup, it sounds good. Feel free to open a PR. I think we also need to make sure that if start_time and end_time are the same, the user is not able to save it. If any change is required, then we will discuss it in the PR thread. @kshitij01042002 @seanlip what do you say?

[yungmulah]

Thanks @shricodev, opened a PR.

[seanlip]

Thanks @shricodev @yungmulah. Re the question about equality, could you please link to the line of code that produced the nonStrictValidationError message in the original description so that I can take a look at it and give you a clear answer here? We should make sure that our validation is fully aligned in all the places which use it.

Thanks!

[yungmulah]

I tried looking into where the error was produced but could not find it. My solution validates on the front end so that the error does not trigger. Maybe @shricodev knows where it is raised?

[yungmulah]

@seanlip @shricodev Is the solution proposed in the PR sufficient for now since it fulfills all the requirements in the Exprected Behaviour of this issue?

[seanlip]

@yungmulah I think you need to address the comment I posted previously. Where is the current message coming from? We do need to ensure consistency.

Have you tried searching the codebase for (parts of) the currently-displayed error message?

[yungmulah]

@seanlip I see, looked some more and found the autosave-info-modals to contain

showNonStrictValidationFailModal(): void {
const modelRef = this.ngbModal.open(
  SaveValidationFailModalComponent, {backdrop: true});
modelRef.result.then(() => {
  this._isModalOpen = false;
}, () => {
  this._isModalOpen = false;
});

this._isModalOpen = true;
}

and also the save-validation-fail-modal.component.html to be raised.

Maybe this can help guiding to exactly where the validation is made?

[seanlip]

@yungmulah That doesn't really look related to the nonStrictValidationError message. But that message has to be coming from somewhere...

Here's what you can do. Try reproducing the original issue. When the error happens, look at (a) the console logs, and (b) the server logs in the terminal. Try pasting them in this thread. Then let's see if there are any clues there?

[yungmulah]

So after reproducing the error I get the following error logs in the console
>PUT http://localhost:8181/createhandler/autosave_draft/6J5KIsxa8wR9 400 (Bad Request)
>logger.service.ts:59 nonStrictValidationFailure: [{"cmd":"edit_state_property","new_value":{"html":"<oppia-noninteractive-video _nghost-mgf-c49=\"\" autoplay-with-value=\"true\" end-with-value=\"5\" ng-version=\"11.2.14\" start-with-value=\"12\" video_id-with-value=\"&amp;quot;cRWUjdymnYI&amp;quot;\"></oppia-noninteractive-video>","content_id":"content_0"},"old_value":{"html":"","content_id":"content_0"},"property_name":"content","state_name":"Introduction"}]

Inspecting the NonStrictValidationError from the console I can see that autosaveChangeListOnChange in change-list.service.ts raises it

autosaveChangeListOnChange(explorationChangeList) {
    // Asynchronously send an autosave request, and check for errors in the
    // response:
    // If an error is present -> Check for the type of error that occurred
    // (Display the corresponding modals in both cases, if not already
    // opened):
    // - Changes are not mergeable when a version mismatch occurs.
    // - Non-strict Validation Fail.
    this.explorationDataService.autosaveChangeListAsync(explorationChangeList, response => {
        if (!response.changes_are_mergeable) {
            if (!this.autosaveInfoModalsService.isModalOpen()) {
                this.autosaveInfoModalsService.showVersionMismatchModal(explorationChangeList);
            }
        }
        this.autosaveInProgressEventEmitter.emit(false);
        if (!response.is_version_of_draft_valid && response.changes_are_mergeable) {
            this.windowRef.nativeWindow.location.reload();
        }
    }, () => {
        this.alertsService.clearWarnings();
        this.loggerService.error('nonStrictValidationFailure: ' + JSON.stringify(explorationChangeList));
        if (!this.autosaveInfoModalsService.isModalOpen()) {
            this.autosaveInfoModalsService.showNonStrictValidationFailModal();
        }
        this.autosaveInProgressEventEmitter.emit(false);
    });
}

which I think is using the exploration-data.service.ts to send data to the exploration editor backend. Unfortunately I canLooking inside the ExplorationDataService I can not seem to find any validation, so maybe this is performed in the backend? Sorry if I am looking in the wrong files, first time doing this @seanlip.

[seanlip]

The >PUT http://localhost:8181/createhandler/autosave_draft/6J5KIsxa8wR9 400 (Bad Request) indicates a backend error. You'll need to look at your server logs (in the terminal, possibly scroll up to see them) to get the python stacktrace and narrow down its source.

[yungmulah]

Thanks for the tip @seanlip, after more investigation this is the error printed in the console from the backend

ERROR:root:Exception raised at http://localhost:8181/createhandler/autosave_draft/Vojxaqt8HWV9: Start value should not be greater than End value in Video tag.
Traceback (most recent call last):
  File "/Users/administrator/Desktop/open-source/oppia/core/controllers/editor.py", line 1478, in put
    exp_services.create_or_update_draft(
  File "/Users/administrator/Desktop/open-source/oppia/core/domain/exp_services.py", line 3113, in create_or_update_draft
    updated_exploration = apply_change_list(exp_id, change_list)
  File "/Users/administrator/Desktop/open-source/oppia/core/domain/exp_services.py", line 880, in apply_change_list
    raise e
  File "/Users/administrator/Desktop/open-source/oppia/core/domain/exp_services.py", line 530, in apply_change_list
    content.validate()
  File "/Users/administrator/Desktop/open-source/oppia/core/domain/state_domain.py", line 3487, in validate
    html_cleaner.validate_rte_tags(self.html)
  File "/Users/administrator/Desktop/open-source/oppia/core/domain/html_cleaner.py", line 404, in validate_rte_tags
    raise utils.ValidationError(
core.utils.ValidationError: Start value should not be greater than End value in Video tag.

which takes me to html_cleaner.py in the backend that does the following validation on line 401-406

start_value = float(tag['start-with-value'].strip())
end_value = float(tag['end-with-value'].strip())
if start_value > end_value and start_value != 0.0 and end_value != 0.0:
     raise utils.ValidationError(
            'Start value should not be greater than End value in Video tag.'
            )

so I guess this is what causes the NonStrictValidationError when using a start time larger than the end time.

[yungmulah]

Hi again @seanlip, was this the code you were looking for?

[seanlip]

Thanks @yungmulah, sorry for the late reply (I was travelling and just got back). Yes, that code is the correct one. Please change it to prevent equality as well, so that the frontend and backend code are fully consistent. Thanks!

[yungmulah]

Thanks @seanlip, added the = sign so now frontend and backend code should be fully consistent. Opened a PR, PTAL.

[AFZL210]

@yungmulah are you still working on this issue?

[yungmulah]

@AFZL210 yes I have a PR open for it waiting to be accepted

[seanlip]

Deassigning @yungmulah due to lack of response.

To new contributors: a lot of progress has been made in #19179 already. Please take a look at that PR and make the final changes needed. To claim this issue, you should demonstrate that those changes work and explain how you addressed the open review comments. Thanks!

Note: it's probably also worth addressing #18508 (comment) similarly if it's still an issue (i.e., don't allow SkillReview components to be saved if the relevant fields aren't populated).

[alice21mota]

Hello! @seanlip
I see this issue has been inactive for a while. If possible, I would like to be assigned to it.
This is the solution I propose, very similar to the one presented by @yungmulah.
The changes I would made would be in the following files: core/templates/services/rte-helper-modal.controller.ts and core/templates/services/rte-helper-modal.component.html. In the first, I would create a disableSaveButtonForVideo function, that would verify if end time is bigger than start time, and if not, disable the button. In the latter file, I would simply apply that condition to the button, raising an error if the values are invalid.
Does this seem ok?
Thank you for your attention.

[alice21mota]

Here is a video of the implementation. When start is equal or greater than end, and the user tries to click "Done", an error is displayed to the user. Clicking "Done" and saving is only possible when the condition is met.

Screen.Recording.2024-03-23.at.13.10.44.mov

[seanlip]

Hi @alice21mota, thanks for looking into this issue. Please take the PR from @yungmulah as your starting point, it has already been refined through a lot of comments and we should get that in properly. Instead of having a new approach as you do here, could you please iterate on that previous approach and address any remaining comments so that you have something that works properly and is generalizable? Thanks!

[alice21mota]

Hello @seanlip, thank you so much for your answer!
Would it be possible to specify which are those remaining comments? The PR from @yungmulah was one of the latest comments, so I'm not understanding what is missing.
Right now, this solution is working as in the video, raising erros if end time is not greater than start time. What changes would you like me to make?
Thank you.

[seanlip]

@alice21mota I mean the remaining comments in #19179 -- I would advise starting from that PR as a basis and fixing whatever's left from the most recent review, instead of your proposed approach.

[alice21mota]

Thank you @seanlip! Will do that, and propose a solution as soon as possible.

[joanapeixinho]

Hello @seanlip! I started from @yungmulahand 's PR and fixed the few things that were remaining. Here is the video with the bug fixed. What should I do next? Should I open a PR? Thanks for the help in advance :)

Oppia.-.Google.Chrome.2024-03-24.17-43-47.mp4

[seanlip]

Hi @joanapeixinho -- yes, feel free to open a PR for this. Also, please reply to the comments in #19179, linking to your PR and explaining how you addressed the comment (or just saying "Done" if it's straightforward). Thanks!

[seanlip]

@joanapeixinho I think you left a comment asking about where to leave questions, but I can't see it any more. The answer is that you can leave that question anywhere, either here or in the other PR. I think I'd probably suggest doing it here with a clear link to the specific comment on the other PR, so that it remains part of this conversation.

Thanks!

[seanlip]

Also @joanapeixinho just to check, when will you be able to open the PR?

[joanapeixinho]

@seanlip I just have to resolve a problem I'm having with the frontend tests that I think is due to my virtual enviroment. It's actually taking some more time than I anticipated since I also have uni classes but hopefully by tomorrow I can have everything solved and do the PR.