[#12557] Instructor edit feedback session page: NumberFormatException when inputting decimal numbers into distribute points questions

[dlimyy]

Fixes #12557

Outline of Solution

Similar to the numerical scale question, I made it such that the number inputted will always be an integer so if the user tries keying in a number such as 99.5, it will be rounded up to 100. This helps to prevent this error from occurring and also reduces amount of code which has to be changed to accommodate decimals.

Screen recording of solution
https://github.com/TEAMMATES/teammates/assets/97420966/a311869e-e8e0-40a7-8724-cc568cb9e27d

[weiquu]

Hi @dlimyy, fix looks good! I would suggest shifting the ceil implementation into the QuestionEditDetailsFormComponent base class (question-edit-details-form.component.ts), since it now can be used for 3 different question types. Then, you can delete the 2 functions in the constsum classes, as well as edit the numscale html file to use the ceil function.

[dlimyy]

Thank you for reviewing my PR and sorry for the delay in making these changes. I have shifted ceil's implementation to the base class and edited numscale's html file.

[dlimyy]

Ready for review

[samuelfangjw]

Thanks for taking the time to contribute to TEAMMATES!

Solution definitely works, but I have a little gripe with this. Notice how input validation for this field is handled in two separate ways.

1. For decimal point, we round the number only when the model is updated.
2. For other non-numeric symbols and letters, we do not allow them to be entered at all.

Though this is but a minor UX inconsistency, I think we can do better. I'm proposing we handle input validation using method 2 as follows.

<input type="number" (keypress)="onPointsInput($event)" />

onPointsInput(event: KeyboardEvent) {
  const { key } = event;
  const isBackspace = key === "Backspace";
  const isDigit = /[0-9]/.test(key);

  if (!isBackspace && !isDigit) {
    event.preventDefault();
  }
}

This will prevent non-numerical characters from being entered in the first place, with the bonus of preventing negative numbers and other potential edge cases we have yet to find as well.

Do let me know your thoughts on this.

[samuelfangjw]

Let's remove step property, default value is already 1.

[samuelfangjw]

Suggested change

	/**
	* Rounds up and returns the smallest integer greater than or equal to
	* the given number if not null
	*/
	/**
	* Rounds up and returns the smallest integer greater than or equal to
	* the given number if not null.
	*/

[samuelfangjw]

Strictly speaking, since strict mode is enabled in tsconfig number is a non-nullable type. This should, if possible, be number | null instead.

[dlimyy]

Thanks for reviewing my PR and sorry for the delay in response. I agree with you that we should be more consistent in the behaviour of the input validation so I have adopted the (keypress) solution mentioned above. However, I discovered that this solution does not prevent the user from pasting decimals numbers in. To resolve this, I implemented the onPaste method which restricts the content that can be pasted into the fields to be strictly numbers only.

List of changes made from previous PR:

• Added the onPointsInput method as suggested above
• Added the onPaste method to prevent non-numerical inputs from being pasted in
• Added max in html files to prevent user from clicking the number spinner wheel to a number that will cause the error mentioned in the issue

[dlimyy]

I have added the tests for onPointsInput to the classes that use this method(since the original class it is implemented in is an abstract class) but currently facing some difficulties in adding the test for the onPasteMethod. When trying to write the test for the method(as shown below), I constantly face the error ReferenceError: ClipboardEvent is not defined. I am not really able to find a solution to this and this error does not occur for other events such as KeyboardEvent and MouseEvent.

 it('should prevent alphabetical input in onPaste', () => {
    const event : ClipboardEvent = new ClipboardEvent('paste');

    event.clipboardData?.setData('text/plain', 'abc');
    const preventDefaultSpy = spyOn(event, 'preventDefault');
    component.onPaste(event);
    expect(preventDefaultSpy).not.toHaveBeenCalled();
  });

I am wondering if anyone knows how to resolve this error. Thanks!

[cedricongjh]

hi @dlimyy, I've looked into the issue, and I found that ClipboardEvent is unfortunately not accessible by the test environment.

A solution I've found to mock a paste event is as follows:

const inputElement = fixture.debugElement.query(By.css('#max-value')).nativeElement as HTMLInputElement;
inputElement.value = 'abc';
expect(inputElement.value).toBe('')

inputElement.value = '2333';
expect(inputElement.value).toBe('2333')

Do try it out and let me know if it works!

[dlimyy]

Hi @cedricongjh, thanks for your help and suggestion! I have tested this out on my end but it does not seem to invoke the onPaste method in the process. If I changed inputElement.value = "2.2", it returns 2.2 instead of ''. I believed the reason why it worked for the 'abc' case is that the input type of max-value is number so any alphabetical characters passed in would get cancelled out. Since the node.js testing environment does not provide support for this ClipboardEvent, would it ok in this case to leave this method untested?

[cedricongjh]

Hi @cedricongjh, thanks for your help and suggestion! I have tested this out on my end but it does not seem to invoke the onPaste method in the process. If I changed inputElement.value = "2.2", it returns 2.2 instead of ''. I believed the reason why it worked for the 'abc' case is that the input type of max-value is number so any alphabetical characters passed in would get cancelled out. Since the node.js testing environment does not provide support for this ClipboardEvent, would it ok in this case to leave this method untested?

Ah that's unfortunate, let's leave that untested for now

[cedricongjh]

Hey @dlimyy, seems like i'm able to enter very large numbers:

Do let me know if you can replicate this on your end and fix it!

[dlimyy]

Hi @cedricongjh, sorry for the delay in getting back to you! I am also able to replicate this bug on my end so currently I am working on a solution which restricts the length of the number to 9 characters to eliminate the chance of the bug occurring

[dlimyy]

I have added my solution which restricts the user from keying in a number that has a length greater than 9 characters. The assignment of target.value is needed to ensure that the number displayed will be correct though the actual points in the model is correct. Could I check if my solution looks ok? I will work on adding the tests for these 3 functions.

[cedricongjh]

I have added my solution which restricts the user from keying in a number that has a length greater than 9 characters. The assignment of target.value is needed to ensure that the number displayed will be correct though the actual points in the model is correct. Could I check if my solution looks ok? I will work on adding the tests for these 3 functions.

hey @dlimyy I think it'll be much better if we added the checking for numbers that are too long in the onPointsInput method, we could do this by accessing the value of the input element like this:

(event.target as HTMLInputElement).value

[dlimyy]

Hi @cedricongjh, thank you for your suggestion. I have initially considered and tried implementing this in onPointsInput but I noticed that this would have a few issues. Firstly, if the user highlights a 9-digit number and types a number over it, the expected behaviour is that the highlighted/selected region is replaced with the number. However, if implemented in onPointsInput, only the last number of the 9 digit number would change rather than the whole highlighted region. Another issue would be that this does not prevent the user from pasting a larger input(Implementing this in the onPaste region with the highlighted region is also quite complicated).

As I noted that my previous solution was quite clunky(with the methods implemented in the individual components), I decided to instead abstract it out into the question-edit-details-form.component(similar to what I did for onPointsInput and onPaste) and found a way for it to update the model and display correctly while ensuring that the aforementioned highlighting problem does not occur. This helped to reduce code duplication and ensure that the amount of code added is minimal(just the restrictPointsLength method and minor changes in html files)

I have also added the tests for the restrictPointsLength method as well.

[cedricongjh]

LGTM, thank you for the work on this!

[jasonqiu212]

Hi @dlimyy, Sorry for the late review! The fix looks good for the 2 distribute points questions. I do have some reservations for the numeric scale question.

I think we still want to cap the inputs, due to the problems big numbers can cause in the backend. But I'm still not too sure what's a good solution. I'll try to come up with one in the next few hours. Feel free to suggest ideas too!

[dlimyy]

Hi @jasonqiu212, thanks for reviewing my PR. For the numerical scale question case, would changing the stepper range to min = 1 and max = 999999999 be a solution since it ensures that the range of the stepper is within value and if the user selects something less than minimum value in the maximum value box, there is an existing error message to catch this.

[jasonqiu212]

Hi @dlimyy, Ah yes, I think that's a valid solution. We can indeed just make use of the existing error message

[dlimyy]

Hi @jasonqiu212, I have implemented this solution to fix this bug. I realised I put the min value twice for the num-scale question so I have removed it.

[jasonqiu212]

Edit: Sorry, accidentally ticked approved

[jasonqiu212]

Hi @dlimyy, I apologize for the excessive back and forth on this PR! Really a big props to you for taking on this tough issue!

I mainly have 1 more suggestion on the increment step input for the numerical scale question. Please let me know what you think or any considerations you may have too!

[dlimyy]

Hi @jasonqiu212, I have added the changes mentioned above. I have also added a new method called onFloatInput as it is needed to prevent the user from keying in the scientific notation e.

[jasonqiu212]

LGTM! Good catch on preventing users from entering e into float inputs! Great work @dlimyy 👍

[jasonqiu212]

Just a note for future reference, custom methods restrictIntegerInputLength and restrictFloatInputLength were necessary to restrict the number of characters a user can input for the following reasons:

• maxlength attribute for <input> does not work type="number"
• min and max attributes for <input> were not sufficient, as the restrictions only apply when clicking on the stepper (i.e. User can still input numbers greater than max)