phetsims / beers-law-lab

"Beer's Law Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/beers-law-lab
GNU General Public License v3.0
2 stars 9 forks source link

Loading Studio with some nonexistent locales results in assertion errors #320

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip)

Operating System 13.1

Browser safari and chrome

Problem description For https://github.com/phetsims/qa/issues/889, when appending the url for studio with a nonexistent locale, such as ty or tt, studio fails to load in English. Instead, I see assertion errors. This doesn't happen with all nonexistent locales--xx, xy, yy, oo --work correctly (the bad locale is ignored and studio opens in English).

This isn't specific to BLL. Also seen with latest pH Scale.

Steps to reproduce Add ?locale=ty to studio url

Visuals

Screenshot 2023-02-01 at 10 00 57 AM Screenshot 2023-02-01 at 10 01 15 AM
samreid commented 1 year ago

@zepumph and I addressed the bug and will commit momentarily. The problem is that it was choosing locales based on what is allowed based our schema, rather than whether there are actually translations for that language.

samreid commented 1 year ago

Fixed, closing. We will discuss whether this needs to be cherry-picked into ph-scale: https://github.com/phetsims/ph-scale/issues/281

pixelzoom commented 1 year ago

Reopening to confirm in https://github.com/phetsims/qa/issues/894. It looks like changes I made during dev testing did not get into the 1.7 release branch.

@KatieWoe can you please test to see if this looks correct in 1.7.0-rc.1.

@samreid @matthew-blackman FYI.

pixelzoom commented 1 year ago

This is not fixed in the 1.7 release branches. With URL http://localhost:8080/studio/?sim=beers-law-lab&locales=*&keyboardLocaleSwitcher&phetioWrapperDebug=true&phetioElementsDisplay=all&locale=tt

assert.js:28 Uncaught Error: Assertion failed: validation failed: Property value not valid: value not in validValues: tt prunedValidator: [object Object] at window.assertions.assertFunction (assert.js:28:13) at validate (validate.ts:25:17) at ReadOnlyProperty.ts:205:34 at LocaleProperty.link (ReadOnlyProperty.ts:407:5) at new ReadOnlyProperty (ReadOnlyProperty.ts:205:12) at new Property (Property.ts:17:5) at new LocaleProperty (localeProperty.ts:45:1) at localeProperty.ts:58:24

This might have been tested for ph-scale, but I don't see any evidence that QA tested for BLL.

It was a fix in joist, and the 1.7 branch contains an old joist sha from 1/24/2023, 6 days before the fix, and the same sha as the 1.7. dev test.

arouinfar commented 1 year ago

The conclusion in https://github.com/phetsims/ph-scale/issues/274 was that there are no longer assertion errors for bad locales, regardless of the state of phetioDebug. This resulted in a change to the PhET-iO Guide which will need to be cherry-picked into the BLL and Concentration release branches.

https://github.com/phetsims/phet-io-sim-specific/commit/b3b518e231cfa737aa5e51b6533647c2f3f7ca11

pixelzoom commented 1 year ago

@KatieWoe @Nancy-Salpepi please reconfirm in 1.7.0-rc.2 for https://github.com/phetsims/qa/issues/894 and https://github.com/phetsims/qa/issues/895.

Nancy-Salpepi commented 1 year ago

This is fixed in 1.7.0-rc.2 for BLL and Concentration. Bad locales are ignored and there are no assertion errors in the console. Keeping open for https://github.com/phetsims/beers-law-lab/issues/320#issuecomment-1421106670.

pixelzoom commented 1 year ago

I guess I'll handle the remaining cherry-pick that's identified in https://github.com/phetsims/beers-law-lab/issues/320#issuecomment-1421106670. The commit is https://github.com/phetsims/phet-io-sim-specific/commit/b3b518e231cfa737aa5e51b6533647c2f3f7ca11.

pixelzoom commented 1 year ago

@jessegreenberg After @arouinfar identified the remaining cherry-pick in https://github.com/phetsims/beers-law-lab/issues/320#issuecomment-1421106670, I see that you made a commit referencing this issue. The commit is https://github.com/phetsims/joist/commit/48a9d9aa11130504d35e34c98e3851607fd83599, and it was made yesterday. It appears to have only been committed to master, and not the beers-law-lab 1.7 branch. Does it need to be patched into 1.7?

Mentioning @samreid too, because his ID appears next to the commit above.

pixelzoom commented 1 year ago

@jessegreenberg I'm now doubly confused because https://github.com/phetsims/joist/commit/48a9d9aa11130504d35e34c98e3851607fd83599 was made to joist branch 'friction-1.6'.

jessegreenberg commented 1 year ago

https://github.com/phetsims/joist/commit/48a9d9aa11130504d35e34c98e3851607fd83599 is just a cherry-pick of https://github.com/phetsims/joist/commit/4889cfb3ae2b4a5ab9cdfdfc948a4b3660140cc5 for the 1.6 release of friction in https://github.com/phetsims/friction/issues/334.

pixelzoom commented 1 year ago

PhET-iO Guide change https://github.com/phetsims/phet-io-sim-specific/commit/b3b518e231cfa737aa5e51b6533647c2f3f7ca11 was cherry-picked to 1.7 branches for beers-law-lab and concentration in the above commits. After building, I verified that this new sentence appears in the PhET-iO Guide:

If you specify a locale for which a translation does not exist, the locale query parameter is ignored and the simulation will retain the localeProperty used when creating the Standard PhET-iO Wrapper.

pixelzoom commented 1 year ago

Verification in the next RC should include the following for beers-law-lab and concentration:

stemilymill commented 1 year ago

looks good