Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

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

Merged
merged 2 commits into from Sep 27, 2021

Conversation

eyelidlessness
Copy link
Contributor

@eyelidlessness eyelidlessness commented Sep 21, 2021

This fixes an issue introduced between this change and this commit in #293, where once again display of binary defaults was broken. This is a minor change to remove behavior which changed a survey's model to use absolute URLs rather than the URLs originally provided to the transformer in the media mapping.

The bulk of the rest of this PR is backfilling tests for getFormParts. I thought that was more thoroughly tested (and there may be some redundancy with last-saved.spec.js), but I didn't feel comfortable writing the relevant test nor making this change without more thorough testing of the function itself.

And, since the regression is present in both 2.8.1 and 3.0.0, we will likely need to patch both. Tomorrow I will also update #291 with the same changes as the regression is present there too.

.eslintrc.json Outdated
"space-before-function-paren": ["error", "never"],
"space-before-function-paren": ["error", {
"anonymous": "never",
"async": "always",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this so you don't end up with async() =>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The others are to preserve the existing rules enforcing spacing in parentheses (which we've discussed moving away from, but that definitely is out of scope here 🙂 ).

Edit: wait that's not right. It's preserving function foo() {} rules which we should preserve, that's a more idiomatic style.

let theme;

beforeEach( async () => {
if ( !Object.prototype.hasOwnProperty.call( settings, 'basePath' ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand how we end up with this being conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both this and the other one you asked about were encountered in the course of writing these tests, and sinon won't stub a nonexistent property. I've tried to trace through how settings is populated but it's quite a winding path and it felt more efficient to just say "because sinon said it wasn't there" 🙁

it( 'falls back to the configured default theme', async () => {
theme = 'configured';

if ( !Object.prototype.hasOwnProperty.call( settings, 'defaultTheme' ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not sure I understand this.

* @param {string} [enketoId]
* @return {string}
*/
const getTransformURL = ( basePath, enketoId ) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to become public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not being exported. For context (sorry), we've been gradually moving toward inline export statements rather than _ prefixes to signify module-private. Besides being a more idiomatic style, it also significantly improves the ability to navigate references across modules.

Copy link

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but the build is broken. As I said to @eyelidlessness, I haven't had a chance to try out the fix manually yet as I'm still getting an env put together, but I trust that's been tried out.

Copy link
Member

@yanokwa yanokwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving based on @seadowg's review.

@yanokwa yanokwa merged commit 63c2dfc into enketo:master Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants