Fix Issue #19910: Changed InputMode for float editor to decimal #20007

Ahmed-Bhouri · 2024-03-20T22:58:28Z

Overview

This PR fixes Issues with inputting Decimal point in Lesson 4 of Financial Literacy #19910.
This PR does the following: Changes the input mode from numeric to decimal so that the decimal point appears on the IOS keyboard. and updates input to accept the keyboard's default decimal point depending on the course language.

Essential Checklist

The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have followed the instructions for making a code change.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

Proof that changes are correct

after the changes the comma finally appears on the keyboard while using the input:

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-03-23.at.14.26.56.mp4

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-03-24.at.20.22.14.mp4

Simulator.Screen.Recording.-.iPhone.15.-.2024-03-26.at.11.36.19.mp4

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.

oppiabot · 2024-03-20T22:58:29Z

Hi @Ahmed-Bhouri, can you complete the following:

The body of this PR is missing the checklist section, please update it to include the checklist.
The proof that changes are correct has not been provided, please make sure to upload a image/video showing that the changes are correct. Or include a sentence saying "No proof of changes needed because" and the reason why proof of changes cannot be provided.
Thanks!

oppiabot · 2024-03-20T22:58:39Z

Hi @Ahmed-Bhouri please assign the required reviewer(s) for this PR. Thanks!

Ahmed-Bhouri · 2024-03-20T22:59:47Z

@seanlip PTAL

oppiabot · 2024-03-20T23:02:51Z

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

seanlip · 2024-03-22T11:16:59Z

@Ahmed-Bhouri Please reassign me once the discussion on the issue is completed and we are clear on the path forward. Thanks!

Ahmed-Bhouri · 2024-03-24T13:36:21Z

@seanlip PTAL

I have finally solved the issue as we discussed.

Sorry for the delay ( I had issues with pushing my code that turned out to be caused by my setup ( This is my first PR and I am running my dev environment on a codespaces instance) )

oppiabot · 2024-03-24T13:39:25Z

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

seanlip · 2024-03-24T14:19:22Z

@Ahmed-Bhouri Thanks -- could you please show the full flow, including submission of the answer? Ideally demonstrate it for both the "." and "," cases. Thanks!

(Also, just a question -- I'm not familiar with Codespaces, but does it run the pre-push hook? Just wanted to check since that can help avoid CI errors in the PR.)

Ahmed-Bhouri · 2024-03-24T19:49:54Z

@seanlip PTAL -- Thank you for mentioning the submission part ( I had to fix a small error there )
I think I am done now I added the video.

I only use codespace to create a separate environment from my machine in the cloud ( I ran into a lot of problems with my 16Gb of ram on a MacBook M1 ). This way I can use better resources.

oppiabot · 2024-03-24T19:52:58Z

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

seanlip · 2024-03-25T16:14:13Z

@Ahmed-Bhouri Thanks, just a question. In your video I don't see the language/region change or the keyboard used to type "2.5". Can you display this too?

seanlip · 2024-03-25T16:15:13Z

core/templates/components/forms/schema-based-editors/schema-based-float-editor.component.ts

 );
 if (error !== undefined) {
 this.localValue = null;
 this.errorStringI18nKey = error || null;
 } else {
 // Parse number if the string is in proper format.
 this.localValue = this.numberConversionService.convertToEnglishDecimal(
- this.localStringValue
+ this.localStringValue.replace(/\.|,/g, currentDecimalSeparator)


A question: Instead of doing localStringValue.replace() in multiple places, should we just be storing the right thing in there from the outset?

Thank you for this!
I have improved it in the commit i am pushing now

oppiabot · 2024-03-30T13:45:37Z

Unassigning @Lawful2002 since they have already approved the PR.

oppiabot · 2024-03-30T13:45:41Z

Hi @Ahmed-Bhouri, 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!

seanlip · 2024-03-30T14:04:52Z

@Ahmed-Bhouri The flake in the last merge attempt seems to be related to your changes. Could you PTAL and analyze the root cause so that we can be sure that this PR does not result in a flake in develop?

https://github.com/oppia/oppia/actions/runs/8491174975/job/23262916789

Chrome Headless 123.0.6312.58 (Linux x86_64) Schema based float editor component should not show error for different decimal points FAILED
	Error: Expected null to equal 12.5.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13368:38)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
	Error: Expected 'I18N_INTERACTIONS_NUMERIC_INPUT_NO_INVALID_CHARS' to equal null.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13369:46)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
	Error: Expected null to equal 12.5.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13372:38)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
	Error: Expected 'I18N_INTERACTIONS_NUMERIC_INPUT_NO_INVALID_CHARS' to equal null.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13373:46)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
Chrome Headless 123.0.6312.58 (Linux x86_64): Executed 319 of 8968 (1 FAILED) (0 secs / 2.741 secs)
Chrome Headless 123.0.6312.58 (Linux x86_64) Schema based float editor component should not show error for different decimal points FAILED
	Error: Expected null to equal 12.5.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13368:38)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
	Error: Expected 'I18N_INTERACTIONS_NUMERIC_INPUT_NO_INVALID_CHARS' to equal null.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13369:46)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
	Error: Expected null to equal 12.5.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13372:38)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)
	Error: Expected 'I18N_INTERACTIONS_NUMERIC_INPUT_NO_INVALID_CHARS' to equal null.
	    at <Jasmine>
	    at UserContext.<anonymous> (core/templates/combined-tests.spec.js:13373:46)
	    at ./node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (core/templates/combined-tests.spec.js:789599:30)
	    at ./node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (core/templates/combined-tests.spec.js:789084:43)

Ahmed-Bhouri · 2024-03-31T02:16:27Z

@seanlip PTAL

To solve this I started by locking the test seed to 505 (the one that fails) to be able to debug the tests easily.
I discovered that my test needs more details to test all possible combinations so I fixed that. That made me understand the origin of the test failing. It turns out the flakes are caused by the NumericInputValidationService where any number with commas is considered to be a non valid number so I added it to the valid characters list solving the issue in the test flake and for all cases.

I am pretty positive now that there will be no more flakes in testing.

Thank you,

oppiabot · 2024-03-31T02:19:31Z

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

seanlip

Thanks @Ahmed-Bhouri! LGTM :)

seanlip · 2024-03-31T17:59:46Z

@lkbhitesh07 PTAL as codeowner. Thanks!

lkbhitesh07

LGTM for the code owner file.
Thanks

oppiabot · 2024-04-03T03:06:01Z

Unassigning @lkbhitesh07 since they have already approved the PR.

oppiabot · 2024-04-03T03:06:04Z

Hi @Ahmed-Bhouri, 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!

Change inputmode to decimal in float editor

ddf6c75

Ahmed-Bhouri requested a review from a team as a code owner March 20, 2024 22:58

Ahmed-Bhouri requested a review from Lawful2002 March 20, 2024 22:58

oppiabot bot assigned Ahmed-Bhouri Mar 20, 2024

oppiabot bot assigned seanlip and unassigned Ahmed-Bhouri Mar 20, 2024

seanlip assigned Ahmed-Bhouri and unassigned seanlip Mar 22, 2024

Ahmed-Bhouri added 2 commits March 23, 2024 13:30

Merge remote-tracking branch 'upstream/develop' into inputmode-decimal

26b6bf0

Accept Dot/Comma in float editor

1099ea5

oppiabot bot assigned seanlip and unassigned Ahmed-Bhouri Mar 24, 2024

seanlip assigned Ahmed-Bhouri and unassigned seanlip Mar 24, 2024

Ahmed-Bhouri added 2 commits March 24, 2024 19:31

Change input value to match currentDecimalSeparator before Submit

83770b3

Merge remote-tracking branch 'upstream/develop' into inputmode-decimal

bd1894e

oppiabot bot assigned seanlip and unassigned Ahmed-Bhouri Mar 24, 2024

seanlip reviewed Mar 25, 2024

View reviewed changes

seanlip added this pull request to the merge queue Mar 30, 2024

oppiabot bot unassigned Lawful2002 Mar 30, 2024

oppiabot bot added the PR: LGTM label Mar 30, 2024

oppiabot bot assigned Ahmed-Bhouri Mar 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 30, 2024

Ahmed-Bhouri added 2 commits March 31, 2024 01:59

Fixed Testing Flakes

688d61a

Merge remote-tracking branch 'upstream/develop' into inputmode-decimal

db7f339

Ahmed-Bhouri requested a review from a team as a code owner March 31, 2024 02:09

Ahmed-Bhouri requested a review from lkbhitesh07 March 31, 2024 02:09

oppiabot bot assigned seanlip and unassigned Ahmed-Bhouri Mar 31, 2024

seanlip approved these changes Mar 31, 2024

View reviewed changes

seanlip enabled auto-merge March 31, 2024 17:59

seanlip assigned lkbhitesh07 and unassigned seanlip Mar 31, 2024

lkbhitesh07 approved these changes Apr 3, 2024

View reviewed changes

seanlip added this pull request to the merge queue Apr 3, 2024

oppiabot bot unassigned lkbhitesh07 Apr 3, 2024

oppiabot bot assigned Ahmed-Bhouri Apr 3, 2024

Merged via the queue into oppia:develop with commit 3c3e5fa Apr 3, 2024
75 checks passed

Ahmed-Bhouri deleted the inputmode-decimal branch April 11, 2024 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue #19910: Changed InputMode for float editor to decimal #20007

Fix Issue #19910: Changed InputMode for float editor to decimal #20007

Ahmed-Bhouri commented Mar 20, 2024 •

edited

Loading

oppiabot bot commented Mar 20, 2024

oppiabot bot commented Mar 20, 2024

Ahmed-Bhouri commented Mar 20, 2024

oppiabot bot commented Mar 20, 2024

seanlip commented Mar 22, 2024

Ahmed-Bhouri commented Mar 24, 2024

oppiabot bot commented Mar 24, 2024

seanlip commented Mar 24, 2024

Ahmed-Bhouri commented Mar 24, 2024

oppiabot bot commented Mar 24, 2024

seanlip commented Mar 25, 2024

seanlip Mar 25, 2024

Ahmed-Bhouri Mar 26, 2024

oppiabot bot commented Mar 30, 2024

oppiabot bot commented Mar 30, 2024

seanlip commented Mar 30, 2024 •

edited

Loading

Ahmed-Bhouri commented Mar 31, 2024

oppiabot bot commented Mar 31, 2024

seanlip left a comment

seanlip commented Mar 31, 2024

lkbhitesh07 left a comment

oppiabot bot commented Apr 3, 2024

oppiabot bot commented Apr 3, 2024

Fix Issue #19910: Changed InputMode for float editor to decimal #20007

Fix Issue #19910: Changed InputMode for float editor to decimal #20007

Conversation

Ahmed-Bhouri commented Mar 20, 2024 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Mar 20, 2024

oppiabot bot commented Mar 20, 2024

Ahmed-Bhouri commented Mar 20, 2024

oppiabot bot commented Mar 20, 2024

seanlip commented Mar 22, 2024

Ahmed-Bhouri commented Mar 24, 2024

oppiabot bot commented Mar 24, 2024

seanlip commented Mar 24, 2024

Ahmed-Bhouri commented Mar 24, 2024

oppiabot bot commented Mar 24, 2024

seanlip commented Mar 25, 2024

seanlip Mar 25, 2024

Choose a reason for hiding this comment

Ahmed-Bhouri Mar 26, 2024

Choose a reason for hiding this comment

oppiabot bot commented Mar 30, 2024

oppiabot bot commented Mar 30, 2024

seanlip commented Mar 30, 2024 • edited Loading

Ahmed-Bhouri commented Mar 31, 2024

oppiabot bot commented Mar 31, 2024

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Mar 31, 2024

lkbhitesh07 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 3, 2024

oppiabot bot commented Apr 3, 2024

Ahmed-Bhouri commented Mar 20, 2024 •

edited

Loading

seanlip commented Mar 30, 2024 •

edited

Loading