disable "show answers" feature in production versions

[pixelzoom]

Add something to joist that allows us to determine (at runtime) whether we're running a production version. The main purpose of this is to turn off query parameters that reveal answers.

@jonathanolson suggested adding Sim.isProduction that parses the version number.

[pixelzoom]

@jonathanolson What do you think about this solution?

isProduction: function() { return assert === null; }

[pixelzoom]

Or maybe we don't even need isProduction. Would this be too awful in (for example) URQueryParameters, immediately after the QueryStringMachine.getAll call?

URQueryParameters.showAnswers = !!( assert && URQueryParameters.showAnswers );

[pixelzoom]

Ah.... assert is also stripped out of dev versions. Bummer.

[pixelzoom]

I'm not wild about testing the version identifier. But if all production versions truly followed the same convention (e.g. "1.2.5"), then it would be:

isProduction: function() {
  // A production version identifier is 3 numbers separated by dots, like "1.4.23"
  return /^\d+\.\d+\.\d+$/.test( phet.chipper.version );
}

But would this be a problem for PhET-iO versions? Or research versions?

[jonathanolson]

But would this be a problem for PhET-iO versions? Or research versions?

Definitely for phet-io, potentially other (future) versions. Could consider leaving it up to the Brand object to see (if it is brand-customized), then the 'phet' brand would just check /^\d+\.\d+\.\d+$/, the phet-io brand would check /^\d+\.\d+\.\d+-phetio$/ or include other suffixes.

Alternately, we could add a flag in the build process (that will be complicated for maintenance releases and build-server).

If we're concerned about showing answers, it may be simplest to white-list domain ranges (www.colorado.edu/physics/phet/dev, phettest.colorado.edu, localhost/127.0.0.1, etc.) instead.

[jonathanolson]

URQueryParameters.showAnswers = !!( assert && URQueryParameters.showAnswers );

We could have most dev builds skip the uglify step (so assertions can be turned on), but we'd probably want RCs to uglify.

[pixelzoom]

I'm unclear on a viable way to do this. And now that I've created issues for where this needed to be applied in my sims, I'd like to get it done. So labeling to (re)discuss at developer meeting.

[jonathanolson]

Thoughts on:

Could consider leaving it up to the Brand object to see (if it is brand-customized), then the 'phet' brand would just check /^\d+.\d+.\d+$/, the phet-io brand would check /^\d+.\d+.\d+-phetio$/ or include other suffixes.

[samreid]

What about only enabling these features if the version contains "-dev" or "-rc"?

[jonathanolson]

What about only enabling these features if the version contains "-dev" or "-rc"?

More specific patterns may be helpful, if things like 1.0.0-phetio-rca could ever be potentially used for an 'RCA' organization, etc.

[samreid]

Perhaps checking if the string endsWith -dev.[0-9]+ or -rc.[0-9]+?

[jonathanolson]

That sounds safe enough to me.

[pixelzoom]

3/2/17 dev meeting:
• Parse version identifier for -dev.[0-9]+ or -rc.[0-9]+
• Sim.isDevVersion and Sim.isRCVersion

[pixelzoom]

We have SimVersion, but it's used only by UpdateCheck.

Sim doesn't use SimVersion, it stores the version identifier as a string:

220 this.version = packageJSON.version; // @public (joist-internal)

And there are multiple ways of getting the version identifier:

• Global phet.joist.sim.version
• Global phet.chipper.version if it's a published version (is this true for phet-io?)
• SimVersion.parse, as done by UpdateCheck

After discovering that phet.chipper.version is defined only by published versions, I'm tempted to write:

function isProductionVersion() { return !!phet.chipper.version; }

... but not confident that this would work with phet-io versions.

If I have to parse the version identifier, I'm tempted to add these static functions to SimVersion:

    /**
     * Does the specified version identifier correspond to a dev version?  E.g. '1.0.0-dev.2'
     * @param {string} versionIdentifier
     * @returns {boolean}
     */
    isDevVersion: function( versionIdentifier ) {
      return /^\d+\.\d+\.\d+-dev.[0-9]+$/.test( versionIdentifier );
    },

    /**
     * Does the specified version identifier correspond to an RC (release candidate) version?  E.g. '1.0.0-rc.2'
     * @param {string} versionIdentifier
     * @returns {boolean}
     */
    isRCVersion: function( versionIdentifier ) {
      return /^\d+\.\d+\.\d+-rc.[0-9]+$/.test( versionIdentifier );
    }

... with awful call sites like:

URQueryParameters.showAnswers = URQueryParameters.showAnswers && 
  !(SimVersion.isDevVersion( phet.joist.sim.version ) ||  SimVersion.isRCVersion( phet.joist.sim.version ));

Or I could take the easy way out and add more baggage to Sim:

// See https://github.com/phetsims/joist/issues/406
isShowAnswersEnabled: function() {
  var isDevVersion = /^\d+\.\d+\.\d+-dev.[0-9]+$/.test( this.version ); // e.g. '1.0.0-dev.1'
  var isRCVersion =  /^\d+\.\d+\.\d+-rc.[0-9]+$/.test( this.version ); // e.g. '1.0.0-rc.1'
  return ( isDevVersion || isRCVersion );
}

... with call sites that look like:

URQueryParameters.showAnswers = URQueryParameters.showAnswers && phet.joist.sim.isShowAnswersEnabled();

@jonathanolson @samreid, your input is needed:

• Can we just test if phet.chipper.version is defined?
• If we have to parse the version identifier, where should we get it?
• Where should the parsing functions live?
• What should the call sites look like?

[pixelzoom]

A side issue here is that I no longer have any faith in the consistency of PhET's version identifier. Imo that is because (a) we haven't evolved the syntax and semantics to accommodate phet-io and (b) we've been publishing things (particularly phet-io) that don't conform to the syntax that we originally agreed on.

[samreid]

Can we just test if phet.chipper.version is defined?

I ran Unit rates 1.0.0-dev.64 and saw this in the console:

I tested the production version of Make a Ten and saw:

So it seems we cannot use the existence phet.chipper.version to distinguish between dev/rc/production versions, because we would like to be able to show answers in published dev and rc versions.

• If we have to parse the version identifier, where should we get it?
Use phet.joist.sim.version because the chipper one is only available for built versions.

Where should the parsing functions live?

How about in SimVersion.parse?

What should the call sites look like?

Create a simVersion instance in Sim.js like this:

this.simVersion = SimVersion.parse( packageJSON.version, phet.chipper.buildTimestamp );

Then create phet.joist.sim.simVersion.isDev and phet.joist.sim.simVersion.isRC and phet.joist.sim.simVersion.isDevOrRC then call sites will look like

URQueryParameters.showAnswers = URQueryParameters.showAnswers && phet.joist.sim.simVersion.isDevOrRC;

[samreid]

A side issue here is that I no longer have any faith in the consistency of PhET's version identifier. Imo that is because (a) we haven't evolved the syntax and semantics to accommodate phet-io and (b) we've been publishing things (particularly phet-io) that don't conform to the syntax that we originally agreed on.

In phetsims/chipper#507 (comment) we decided to add the phetio string as metadata into the version number. This conforms to semver as "Additional labels for pre-release and build metadata" and is formalized and documented in getVersionForBrand.js. Please clarify your specific concerns so we can address or discuss them.

[pixelzoom]

Ideally, showAnswers would move to initialize-globals.js, disabled for production versions in one place (joist?), and used by sims via phet.chipper.queryParameters.showAnswers.

[pixelzoom]

It occurred to me that we are already detecting production versions in order to strip out assertions. In initialize-globals.js:

573 var isProduction = $( 'meta[name=phet-sim-level]' ).attr( 'content' ) === 'production';

How about if we promote this to a global variable? I.e.:

window.phet.chipper.isProduction = $( 'meta[name=phet-sim-level]' ).attr( 'content' ) === 'production';

Then to disable showAnswers , e.g. in EqualityExplorerQueryParameters:

var EqualityExplorerQueryParameters = QueryStringMachine.getAll( {

    // Shows answers to challenges in the 'Solve It!' screen. ...
    showAnswers: { type: 'flag' },
   
    ...
} );

EqualityExplorerQueryParameters.showAnswers = 
  EqualityExplorerQueryParameters.showAnswers && !phet.chipper.isProduction;

And perhaps promote showAnswers to a global query parameter, since it's currently used in 7 sims.

The downside of this approach is that, like assertions, this will disable showAnswers in production and dev versions. ("production" in the metadata actually means "built version".) But that may be acceptable. And it seems preferable to using the version identifier to detect production versions.

Labeling for discussion at developer meeting.

[pixelzoom]

We could enable showAnswers in debug.html by looking at phet.chipper.isDebugBuild. This is how assertions are currently enabled in debug.html (see initialize-globals.js, line 576).

E.g.:

EqualityExplorerQueryParameters.showAnswers = 
  EqualityExplorerQueryParameters.showAnswers && 
  ( !phet.chipper.isProduction || phet.chipper.isDebugBuild );

[samreid]

The two preceding comments seem sensible to me, and not too much work, easy to test.

[jonathanolson]

It occurred to me that we are already detecting production versions in order to strip out assertions. In initialize-globals.js:

It looks like that is hard-coded to 'production' in the template, so ANY sim build includes that flag. So the above notes look good as long as we want showAnswers when "it's either not built or is the debug build".

[pixelzoom]

7/12/18 dev meeting:
Use approach in #406 (comment).
Create globals phet.chipper.isProduction and phet.chipper.showAnswers.
@pixelzoom will do this, and use in EqEx.

[pixelzoom]

Thinking about this a bit more... Since we may want to disable more than just the showAnswers query parameter (e.g. 'autoAnswer' in ArithmeticQueryParameter), perhaps we should have a global variable whose value indicates that we're running the the primary built html file. I.e.:

( phet.chipper.isProduction && !phet.chipper.isDebugBuild )

Do other devs think this is a good idea? Suggestion for this global's name?

[samreid]

My initial reaction is that ( phet.chipper.isProduction && !phet.chipper.isDebugBuild ) seems clear enough--it suggests that production builds can be the debug version or not. I don't know that giving a new name to that combination would be helpful. Happy to be proved wrong though!

[jbphet]

If we're going to have two parameters, how about having an object that could also contain a version ID in case we ever need to change it, e.g:

phet.chipper.buildConfig = {
  isProduction: true,
  isDebugBuild: false,
  formatVersion: "1.0"
}

[pixelzoom]

@jbphet suggested:

how about having an object ... phet.chipper.buildConfig

That doesn't address the issue. Call sites would then have to do
( phet.chipper.buildConfig.isProduction && !phet.chipper.buildConfig.isDebugBuild ).

And buildConfig has an entirely different meaning in the chipper implementation. So that specific name would be potentially confusing.

[pixelzoom]

I added phet.chipper.isProduction and phet.chipper.queryParameters.showAnswers to initialize-globals.js. The latter is overridden and set to false when running the primary built html file.

Tested in equality-explorer with development html file (requirejs mode), equality-explorer_en_phet.html, and equality-explorer_all_phet_debug.html. ?showAnswers was ignored when used with equality-explorer_en_phet.html. I also tested ?ea to verify that the behavior is unchanged.

If similar behavior is needed for other query parameters:

(1) If the query parameter is read using QUERY_PARAMETERS_SCHEMA is initialize-globals.js, override it in this function in initialize-globals.js:

  /**
   * Overrides query parameters when running the primary built html file.  Does not affect debug.html.
   * See https://github.com/phetsims/joist/issues/406
   */
  ( function() {
    if ( phet.chipper.isProduction && !phet.chipper.isDebugBuild ) {
      phet.chipper.queryParameters.showAnswers = false;
    }
  }() );

(2) If the query parameter is sim-specific, evaluate the following expression to decide whether to override it: ( phet.chipper.isProduction && !phet.chipper.isDebugBuild ). E.g. in ArithmeticQueryParameters:

if ( phet.chipper.isProduction && !phet.chipper.isDebugBuild ) {
  ArithmeticQueryParameters.autoAnswer = false;
}

[pixelzoom]

All of my sims have been converted to use phet.chipper.queryParameters.showAnswers.

[pixelzoom]

@jonathanolson randomly selected to review.

[pixelzoom]

Some of my sims (the ones with games) will be picking up this change when published. So this needs to be reviewed before then.

[jonathanolson]

The commits and strategy look good to me. Closing.