-
Notifications
You must be signed in to change notification settings - Fork 49
[Issue #6664] Merge two GetSubmissionListExpanded responses #7699
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
[Issue #6664] Merge two GetSubmissionListExpanded responses #7699
Conversation
api/src/legacy_soap_api/grantors/services/get_submission_list_expanded_response.py
Show resolved
Hide resolved
| operation_method = getattr( | ||
| self, to_snake_case(self.operation_config.request_operation_name) | ||
| ) | ||
| if self.operation_config.has_merge_list_response: |
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.
Do we still need this with how the approach was adjusted? I think a few of the file adjustments you made probably aren't needed anymore as well.
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 took it out and it seems like it works fine, so I'll remove it.
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.
Is the has_merge_list_response param needed at all anymore? I don't think it is since that's now just built into the logic of making the response, I think you can remove all references of it / the config that was added.
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.
My concern is
if proxy_response.status_code != 500 and not self.operation_config.has_merge_list_response:
return proxy_response
Because without that, the proxy_response would take priority and we'd never get to a merged response from what I can tell.
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.
Oh wait, maybe you're right. I'm going to check.
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've removed it. I don't think we need that 500 check since we've got the use_simpler on the config.
Summary
Fixes / Work for #6664
Changes proposed
Merge the two responses together for GetSubmissionListExpanded. Take the proxy response, and extract out the application byte data and inject that into a new mtom message with the simpler data added in too.
Context for reviewers
We want when people to hit the GetSubmissionListExpanded endpoint that they get a single response with data from both the proxy and the simpler db.
Validation steps