Fix: resolve `jr:` URLs with spaces #291

eyelidlessness · 2021-07-22T00:22:06Z

Fixes enketo/enketo#1101. Opening as a draft as it depends on enketo/enketo-transformer#111. And branched from #293 (see diff) as a prerequisite to validate image labels.

A few notes about these changes:

This project was not previously using the transformer's media mapping logic, and had reimplemented it redundantly. That code is now removed.
That code previously had additional escaping of & characters, presumably as an attempt to avoid HTML/XML injection. I have not covered that because I don't really know what cases should be covered. But I strongly suspect that responsibility belongs in transformer, and likely is already doing the right thing by using DOM-like methods to set those attributes.
There were only minor changes to transformation-controller, but given so many regressions in untested code in last-saved I decided to be extra cautious and make sure there's at least some test coverage. It's far from exhaustive, but it does cover the most important changes I could identify. The test setup is quite involved, but now that we have more coverage it should help breaking up the functionality into more testable bits.
I've also manually validated the cases I could think of.
There is an outstanding cache invalidation problem. I think it's probably in scope for this PR, but I wanted to get this visible and the discussion started sooner rather than later.

Also brings in the remaining relevant update test from enketo#291

seadowg

I'm a little fuzzy on the responsibility split between transfomer and express so just leaving these comments here for discussion to get the ball rolling.

package.json

seadowg · 2021-09-17T12:27:30Z

app/controllers/transformation-controller.js

+        .then( communicator.getManifest )
+        .then( xform => {
+            const survey = Object.assign( {}, xform, {
+                media: utils.toMediaMap( xform.manifest ),


Thinking out loud: If we end up with an escaped media map on the survey before we pass it to transform, why do we need to then do the escaping in transform?

I think the necessity is the other way around: transform needs to escape for consistency, as it's a public API in a standalone package.

If I remember correctly, this change was introduced before the change in transformer to escape all media keys and values, so it may not be strictly necessary here. But in my opinion it's probably better to also escape here for consistency/correctness.

seadowg · 2021-09-17T12:29:31Z

app/lib/communicator/communicator.js

@@ -62,7 +73,7 @@ function getXForm( survey ) {
 * @return { Promise<module:survey-model~SurveyObject> } a Promise that resolves with a survey object with added manifest
 */
 function getManifest( survey ) {
-    if ( !survey.info.manifestUrl ) {
+    if ( survey.info == null || !survey.info.manifestUrl ) {


What drives out this change?

I don't recall where I hit this, probably in new tests or while manually validating the change. But it's more consistent with the Survey "type" as it exists in reality, and should have been checked here already.

public/js/src/module/connection.js

seadowg · 2021-09-17T12:33:01Z

test/server/utils.spec.js

-            config[ 'base path' ] = 'http://enke.to';
-        });
+        beforeEach( () => {
+            sandbox = sinon.createSandbox();


Could we just pass in the base path to toLocalMediaUrl to avoid this kind of static stubbing, or is that more change than we're comfortable with?

This change was more about test isolation (and consistency of same). There have been several cases where stateful setup/teardown like this has interfered with other tests.

I would absolutely love to make that change (and to generally pass arguments rather than depend on state like config). But I think I'd prefer to avoid major interface changes like that in such a narrowly scoped bugfix PR. With questionable test coverage and the potential for dynamic calls that could be hard to find, I think it's safer to hold off on that.

seadowg · 2021-09-17T12:33:29Z

test/server/utils.spec.js

+        beforeEach( () => {
+            sandbox = sinon.createSandbox();
+
+            sandbox.stub( config, 'base path' ).get( () => 'http://enke.to' );


Same as above around passing the base path in.

seadowg · 2021-09-17T12:52:15Z

test/server/transformation-controller.spec.js

+
+describe( 'Transformation Controller', () => {
+    const basePath = '';
+    const bearer = 'fozzie';


I was wondering when someone would notice my increasingly bold test fixture jokes! I guess this one's more obvious than the David Bowie references. 🤣

seadowg · 2021-09-17T12:54:47Z

test/server/transformation-controller.spec.js

+    beforeEach( done => {
+        sandbox = sinon.createSandbox();
+
+        sandbox.stub( config, 'base path' ).get( () => basePath );


I'm guessing you have to do this here as we're testing a controller (assuming it's basically a black box). Correct me if I'm wrong on that

Unless I'm mistaken about some way it could be parameterized in the API, I think that's correct. (And I hope it is, I'm not sure it would be great if that could be parameterized.)

seadowg · 2021-09-17T12:58:52Z

test/server/transformation-controller.spec.js

+        },
+    ];
+
+    requests.forEach( ( { description, url, body } ) => {


Again not sure about this style but I get that otherwise you end up with repeated tests. I think I'm always worried that people then add tests to the block that only get run as part of one case, and you end up with conditional assertions/tests. It's clean at the moment though 🤷‍♂️

Oh I understand this concern better now that you spelled it out! Yeah, as we've discussed before, I'm similarly cautious about overly "helping" tests. I felt okay with it here because I don't foresee a change which would ever produce conditional behavior to be tested here.

seadowg · 2021-09-17T12:59:09Z

test/server/transformation-controller.spec.js

+            beforeEach( done => {
+                // Stub getXForm
+                const xform = `
+                    <?xml version="1.0"?>


Could this move to a file or even just to the bottom of this file?

There's precedent for external fixture files in core/transformer. In my experience it makes it harder to reason about the tests using them, because you need at least three files open to understand the tested behavior, the state of the test, and the test itself.

I have similar concern about moving it below, but more in the sense that it feels odd to me to have references to something before its definition, and tends to obscure context.

Is the concern that it's large and pushes the actual tests down? I know that may be annoying for this review, but FWIW most editors/IDEs will support collapsing it so it shouldn't be too big a burden going forward.

seadowg · 2021-09-21T17:42:26Z

app/lib/utils.js

@@ -230,9 +231,25 @@ function areOwnPropertiesEqual( a, b ) {
 function toLocalMediaUrl( url ) {


We could split this out to a new file

seadowg

I think this just needs the transform release now so the PR can go green!

Resolve jr: URLs with spaces Address PR feedback: move URL functions to url.js Update dependencies - enketo-transformer - enketo-core - moved node-sass and grunt-sass to devDependencies - all possible npm audit fixes

eyelidlessness requested a review from lognaturel July 22, 2021 00:22

eyelidlessness added a commit to eyelidlessness/enketo-express that referenced this pull request Aug 2, 2021

Fix form cache update

4c8cc15

Also brings in the remaining relevant update test from enketo#291

eyelidlessness added a commit to eyelidlessness/enketo-express that referenced this pull request Aug 2, 2021

Fix form cache update

a367b71

Also brings in the remaining relevant update test from enketo#291

eyelidlessness force-pushed the fix/218-media-url-spaces branch 2 times, most recently from dfa82ed to 7b0995a Compare August 3, 2021 21:47

eyelidlessness added a commit to eyelidlessness/enketo-express that referenced this pull request Aug 17, 2021

Fix form cache update

cd462ed

Also brings in the remaining relevant update test from enketo#291

eyelidlessness added a commit to eyelidlessness/enketo-express that referenced this pull request Aug 17, 2021

Fix form cache update

dc52e78

Also brings in the remaining relevant update test from enketo#291

eyelidlessness force-pushed the fix/218-media-url-spaces branch from 7b0995a to e87a742 Compare September 10, 2021 19:05

yanokwa pushed a commit to yanokwa/enketo-express that referenced this pull request Sep 10, 2021

Fix form cache update

c968f15

Also brings in the remaining relevant update test from enketo#291

yanokwa mentioned this pull request Sep 10, 2021

Handle media filenames containing spaces #268

Closed

seadowg reviewed Sep 17, 2021

View reviewed changes

eyelidlessness mentioned this pull request Sep 17, 2021

Escape spaces in jr: media URLs enketo/enketo-transformer#111

Merged

seadowg reviewed Sep 21, 2021

View reviewed changes

eyelidlessness mentioned this pull request Sep 21, 2021

Fix: rendering of binary defaults in online-only mode #316

Merged

eyelidlessness force-pushed the fix/218-media-url-spaces branch from e87a742 to 419f2d9 Compare September 28, 2021 19:22

seadowg suggested changes Sep 29, 2021

View reviewed changes

eyelidlessness changed the base branch from master to qa/3.0.2-pre October 13, 2021 21:31

yanokwa marked this pull request as ready for review October 13, 2021 21:38

eyelidlessness force-pushed the fix/218-media-url-spaces branch 2 times, most recently from 9e36188 to cd9cc7b Compare October 13, 2021 22:41

Fix: resolve jr: URLs with spaces

0f338b0

Resolve jr: URLs with spaces Address PR feedback: move URL functions to url.js Update dependencies - enketo-transformer - enketo-core - moved node-sass and grunt-sass to devDependencies - all possible npm audit fixes

eyelidlessness force-pushed the fix/218-media-url-spaces branch from cd9cc7b to 0f338b0 Compare October 13, 2021 23:01

lognaturel approved these changes Oct 13, 2021

View reviewed changes

yanokwa merged commit 60ccd21 into enketo:qa/3.0.2-pre Oct 13, 2021

This was referenced Oct 14, 2021

Fix: form fails to load when no media manifest is provided #332

Merged

Changes for 3.0.2 #341

Merged

This was referenced Nov 1, 2021

Annotation of binary default images does not submit the annotated image enketo/enketo#1034

Closed

Fix: ensure dynamic manifest URLs are mapped #360

Merged

eyelidlessness mentioned this pull request Jan 20, 2022

Fix: ensure form_logo.png is included when present in media mapping #380

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: resolve `jr:` URLs with spaces #291

Fix: resolve `jr:` URLs with spaces #291

eyelidlessness commented Jul 22, 2021 •

edited

Loading

seadowg left a comment

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 17, 2021

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 17, 2021

eyelidlessness Sep 17, 2021

seadowg Sep 21, 2021

seadowg left a comment

		@@ -230,9 +231,25 @@ function areOwnPropertiesEqual( a, b ) {
		function toLocalMediaUrl( url ) {

Fix: resolve jr: URLs with spaces #291

Fix: resolve jr: URLs with spaces #291

Conversation

eyelidlessness commented Jul 22, 2021 • edited Loading

seadowg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadowg left a comment

Choose a reason for hiding this comment

Fix: resolve `jr:` URLs with spaces #291

Fix: resolve `jr:` URLs with spaces #291

eyelidlessness commented Jul 22, 2021 •

edited

Loading