Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #901 ...
Nancy-Salpepi opened this issue Feb 1, 2023 · 14 comments
Closed
Tracked by #901 ...

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air (m1 chip)

Operating System
13.1

Browser
safari and chrome

Problem description
For phetsims/qa#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
Copy link
Member

samreid commented Feb 1, 2023

@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
Copy link
Member

samreid commented Feb 1, 2023

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

@samreid samreid closed this as completed Feb 1, 2023
matthew-blackman pushed a commit to phetsims/joist that referenced this issue Feb 3, 2023
matthew-blackman pushed a commit to phetsims/joist that referenced this issue Feb 3, 2023
@pixelzoom
Copy link
Contributor

Reopening to confirm in phetsims/qa#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 pixelzoom reopened this Feb 6, 2023
@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 6, 2023

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
Copy link
Contributor

The conclusion in phetsims/ph-scale#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
Copy link
Contributor

pixelzoom commented Feb 8, 2023

@KatieWoe @Nancy-Salpepi please reconfirm in 1.7.0-rc.2 for phetsims/qa#894 and phetsims/qa#895.

@Nancy-Salpepi
Copy link
Author

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 #320 (comment).

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 8, 2023

I guess I'll handle the remaining cherry-pick that's identified in #320 (comment). The commit is
https://github.com/phetsims/phet-io-sim-specific/commit/b3b518e231cfa737aa5e51b6533647c2f3f7ca11.

@pixelzoom pixelzoom assigned pixelzoom and unassigned KatieWoe Feb 8, 2023
@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 8, 2023

@jessegreenberg After @arouinfar identified the remaining cherry-pick in #320 (comment), I see that you made a commit referencing this issue. The commit is phetsims/joist@48a9d9a, 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
Copy link
Contributor

pixelzoom commented Feb 8, 2023

@jessegreenberg I'm now doubly confused because phetsims/joist@48a9d9a was made to joist branch 'friction-1.6'.

@jessegreenberg
Copy link
Contributor

phetsims/joist@48a9d9a is just a cherry-pick of phetsims/joist@4889cfb for the 1.6 release of friction in phetsims/friction#334.

@jessegreenberg jessegreenberg removed their assignment Feb 8, 2023
@samreid samreid removed their assignment Feb 9, 2023
pixelzoom added a commit to phetsims/concentration that referenced this issue Feb 9, 2023
@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 9, 2023

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
Copy link
Contributor

pixelzoom commented Feb 9, 2023

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

@stemilymill
Copy link

looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants