Skip to content
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

Merged
merged 4 commits into from
Mar 1, 2023

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented Feb 25, 2023

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:

  • run make test-full and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

Copy link
Member

@matthew-white matthew-white left a 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!

docs/api.md Outdated Show resolved Hide resolved
docs/api.md Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
lib/resources/submissions.js Outdated Show resolved Hide resolved
lib/model/query/submissions.js Show resolved Hide resolved
test/assertions.js Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
lib/model/frames/submission.js Show resolved Hide resolved
Comment on lines +156 to +160
// 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`
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

@matthew-white matthew-white left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants