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

Add Enketo version (release tag, or git commit hash, or package.json version) to User-Agent header to server-sent requests #391

Merged

Conversation

eyelidlessness
Copy link
Contributor

@eyelidlessness eyelidlessness commented Feb 16, 2022

Closes enketo/enketo#1021. Only opening as draft because I've done no manual testing, but want to make it available for review.

I have verified this PR works with

  • Online form submission
  • Offline form submission
  • Saving offline drafts
  • Submitting offline drafts
  • Editing submissions
  • Form preview
  • None of the above

What else has been done to verify that this works as intended?

Expanded test coverage of config.version.

Why is this the best possible solution? Were any other approaches considered?

This is probably not the most thorough "version" determination for all scenarios (relevant discussion in #382), but we're hoping to address that. For now, the goal is to reuse the same logic so it can be updated in one place.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

If Enketo is linked to a server which is not expecting this header I suppose it could cause unexpected behavior? But that seems exceedingly unlikely.

Do we need any specific form for testing your changes? If so, please attach one.

N/A

Also use mocks instead of side effects to test various conditions in config. These have already been flaky, but they're especially so when repeatedly testing config-model.js in isolation.
@yanokwa
Copy link
Member

yanokwa commented Feb 17, 2022

When I tried this on the QA server, Enketo wouldn't load because express-cls-hooked was missing. It's marked as a dev dependency, so npm --prune production removes it.

@eyelidlessness
Copy link
Contributor Author

Oof, muscle memory misfire! Fixed.

@eyelidlessness eyelidlessness marked this pull request as ready for review February 17, 2022 18:40
@eyelidlessness
Copy link
Contributor Author

Part of my manual validation step is looking at the User-Agent displayed on submissions in Central. I'd assume that edits should not change the value of the original submission—and it did not change—but I do wonder whether we're also capturing UA on the Central side for edits.

@lognaturel
Copy link
Contributor

lognaturel commented Feb 17, 2022

whether we're also capturing UA on the Central side for edits.

As confirmed by @matthew-white, it's captured and API-accessible, just not in the frontend yet!

version does not account for git refs on forks, which would be an issue when deploying unmerged changes for testing.

I think it can be dev responsibility to keep track of what branches they've tried out. I'm not too worried about this. If it ends up being a pain point we can come back to it.

All of this is looking great to me now. The only last remaining issue I see is that the file written out in the case of no git environment seems to have a trailing newline. It now gets sanitized as a space so it works but it looks a little funky.

@lognaturel lognaturel merged commit b3cab5e into enketo:master Feb 18, 2022
@lognaturel lognaturel changed the title Add User-Agent header Add Enketo version to User-Agent header Feb 18, 2022
@lognaturel lognaturel changed the title Add Enketo version to User-Agent header Add Enketo version (release tag, or git commit hash, or package.json version) to User-Agent header to server-sent requests Feb 18, 2022
@lognaturel
Copy link
Contributor

This caused a memory leak fixed in #450. As @eyelidlessness says there, "Continuation Local Storage (whether using async hooks or not) isn't a great idea in the first place" but it was a pragmatic approach to get this functionality in.

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.

User-Agent string should include Enketo Express version
3 participants