phetsims / chipper

Tools for developing and building PhET interactive simulations.
MIT License
11 stars 14 forks source link

regionAndCulture query parameter is not being validated. #1422

Closed pixelzoom closed 7 months ago

pixelzoom commented 7 months ago

Reported in Slack#x-raisins by @Nancy-Salpepi:

Hi Team! Quick question on region and culture…. I noticed in the Number Line Suite, with ?regionAndCulture that the “Invalid Query Parameters” dialog appears when typing in a nonexistent option. I don’t see that with Arithmetic. Instead the sim doesn’t load and there is an error in the console. I was just wondering what dictates whether the dialog appears or not?

It looks like the regionAndCulture query parameter schema in initialize-globals.ts does not have validValues or isValidValue fields. So the query parameter parser assumes has no way to valid, no way to display a dialog, and it fails further downstream.

pixelzoom commented 7 months ago

In https://github.com/phetsims/chipper/commit/c1663259f4134d9119e117a05d960b56fb142bd4, I added validValues. A potential downside to this approach is that regionAndCulture=usa will always succeed, even if the sim does not support the Region and Culture feature (has no supportedRegionsAndCultures in package.json).

An alternative implementation shown below uses isValidValue, and regionAndCulture=usa (or any value) will fail for sims that do not support the Region and Culture feature.

Which behavior is desired?

Subject: [PATCH] add validValues to regionAndCulture query parameter, https://github.com/phetsims/joist/issues/953
---
Index: js/initialize-globals.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/initialize-globals.js b/js/initialize-globals.js
--- a/js/initialize-globals.js  (revision c1663259f4134d9119e117a05d960b56fb142bd4)
+++ b/js/initialize-globals.js  (date 1710529395526)
@@ -494,7 +494,15 @@
       public: true,
       type: 'string',
       defaultValue: packagePhet?.simFeatures?.defaultRegionAndCulture ?? 'usa',
-      validValues: packagePhet?.simFeatures?.supportedRegionsAndCultures ?? [ 'usa' ] // default value must be in validValues
+      isValidValue: value => {
+        const supportedRegionsAndCultures = packagePhet?.simFeatures?.supportedRegionsAndCultures;
+        if ( supportedRegionsAndCultures ) {
+          return supportedRegionsAndCultures.includes( value );
+        }
+        else {
+          return false;
+        }
+      }
     },

     /**
pixelzoom commented 7 months ago

Oops, nevermind. We cannot do the above isValidValue approach. Because regionAndCulture must have a default value, validation needs to succeed when that default value is checked.

Closing.

pixelzoom commented 7 months ago

Reopening, to have @Nancy-Salpepi verify. Close if OK.

To summarize the expected behavior:

In all cases that fail, a dialog will be shown.

Nancy-Salpepi commented 7 months ago

Everything looks good to me on main! Closing.