-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable submission via ODK Web Forms #1370
base: master
Are you sure you want to change the base?
Enable submission via ODK Web Forms #1370
Conversation
17cac2e
to
a5ac918
Compare
d5adc9f
to
2acd51d
Compare
select ${fields} from forms | ||
left outer join form_defs on ${versionJoinCondition(version)} | ||
${enketoId ? sql`join` : sql`left outer join`} form_defs on ${versionJoinCondition(version, enketoId)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to fetch the correct form_def. Provided enketoId
can belong to the published Form or the draft one.
Another approach I was considering was to create a new function to first find out for which version provided enketoId
belongs and then fetch that Form. If the current change is making already complicated query more complicated then I am open for creating separate query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok with me for now - it's a more succinct change than I expected and like that.
* skip failing migration test as they have already served their purpose
* revert changes to sqlEqual * added a new rest-endpoint to submit using draftToken * Updated API doc
2acd51d
to
84241c2
Compare
select ${fields} from forms | ||
left outer join form_defs on ${versionJoinCondition(version)} | ||
${enketoId ? sql`join` : sql`left outer join`} form_defs on ${versionJoinCondition(version, enketoId)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok with me for now - it's a more succinct change than I expected and like that.
@@ -611,7 +611,7 @@ describe.skip('database migrations from 20230406: altering entities and entity_d | |||
describe('database migrations from 20230512: adding entity_def_sources table', function () { | |||
this.timeout(20000); | |||
|
|||
it('should backfill entityId and entityDefId in audit log', testServiceFullTrx(async (service, container) => { | |||
it.skip('should backfill entityId and entityDefId in audit log', testServiceFullTrx(async (service, container) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming these are skipped because there is something about the application code that has gotten enough out of sync that these migration tests no longer work.
Towards getodk/web-forms#23 and getodk/central#883
Changes
What has been done to verify that this works as intended?
Added tests.
Why is this the best possible solution? Were any other approaches considered?
Generally uses the existing approach. There's one inline comment regarding filtering by EnketoId, I am open to changing that approach.
The URL for the new endpoint to retrieve Form via EnketoId was discuss in the weekly meeting.
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?
None
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
API Doc has been updated as well.
Before submitting this PR, please make sure you have:
make test
and confirmed all checks still pass OR confirm CircleCI build passes