-
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
Update Submissions response #783
Update Submissions response #783
Conversation
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 looking like a nice set of improvements to the API and the docs!
read logical submission from database on submission update
// Helper function to assign submission.currentVersionSubmitter to submission.currentVersion.submitter | ||
// Current there is no way to create such complex object using `extender` and `unjoiner` framework functions | ||
const assignCurrentVersionSubmitter = (x) => x.withAux('currentVersion', x.aux.currentVersion.withAux('submitter', x.aux.currentVersionSubmitter)); | ||
|
||
const _get = extender(Submission, Submission.Def.into('currentVersion'))(Actor.into('submitter'), Actor.alias('current_version_actors', 'currentVersionSubmitter'))((fields, extend, options, projectId, xmlFormId, draft) => sql` |
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 might be wrong here, do you know if there is a way to use extender
to set currentVersion.submitter
?
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 not sure actually! I can't think of a similar example of nested extended metadata, so maybe not.
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'll take a closer look tomorrow, but this is looking good to me. I think we should go ahead and merge so that QA can start on regression testing.
Closes #417, #481, #590
What has been done to verify that this works as intended?
Updated integration tests
Why is this the best possible solution? Were any other approaches considered?
Detailed discussion on Slack
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.md as part of this PR.
Updated.
Before submitting this PR, please make sure you have:
make test-full
and confirmed all checks still pass OR confirm CircleCI build passes