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

feat: filter storage mode responses by submission id #362

Merged
merged 3 commits into from
Sep 28, 2020

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Sep 22, 2020

Fixes a bug with #174 for release. #174 was reverted in #358.

frankchn and others added 2 commits September 22, 2020 16:25
* feat: Filter Storage Mode Responses by Submission Id (updated 2020-09-13)

* Update page design so that it is compatible with tablets and everything is on one row

* Update CSS for iPad
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - could i trouble you to test this in staging to guarantee performance before merging, so that it definitely makes the next release? it's been languishing awhile.

FYI @frankchn, there was a bug with the conditional logic which meant we had to revert this PR during the release process. Return statements are booby traps when used in a callback pattern...

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment on the logging format. I also think it might be worth the effort to extract some of this function into a service, or at least use async/await to make the control flow a little more obvious, but will defer to @liangyuanruo

@karrui karrui self-assigned this Sep 28, 2020
@mantariksh
Copy link
Contributor

fixed the log statement and tested on staging-alt, nothing unusual seems to be happening with the staging-alt db so I think we're good to go

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

Successfully merging this pull request may close these issues.

5 participants