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 #71

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

frankchn
Copy link
Contributor

@frankchn frankchn commented Aug 4, 2020

Problem

Users have trouble finding specific responses and trying to download attachments for these responses.

Solution

This introduces a new textbox which filters submissions by reference ID so users can jump to a certain response immediately.

Screenshots

image

image

image

@frankchn frankchn requested a review from liangyuanruo August 4, 2020 07:46
@frankchn
Copy link
Contributor Author

frankchn commented Aug 4, 2020

Some design help for the "no responses found screen/table" and the exact placement for the filter textbox will be much appreciated.

@jonathangohjy
Copy link

jonathangohjy commented Aug 6, 2020

Have uploaded screens on Zeplin for implementation

Placement of search bar
No Results screen

@frankchn frankchn force-pushed the frank-search-response branch from 4486480 to f65693a Compare August 7, 2020 07:36
@frankchn
Copy link
Contributor Author

frankchn commented Aug 7, 2020

@liangyuanruo can you take a look? this is pretty close -- I just don't have the SVG for the "filter" icon.

@frankchn frankchn changed the title [wip] Filter Storage Mode Responses by Submission Id feat: Filter Storage Mode Responses by Submission Id Aug 7, 2020
@liangyuanruo
Copy link
Contributor

liangyuanruo commented Aug 9, 2020

Hey @frankchn ,

I just tried it out the feature, and noticed a few things

  1. I see @jonathangohjy has introduced the filter button, but I would like to revert the implementation back to a search bar without the filter button instead. The reason is because we expect users to copy-paste the ObjectID into the input box from the downloaded CSV, and we want the search to be immediately responsive, instead of introducing another click for the user interaction - I'd suggest using the ng-change directive here with the <input> element. We'd also need to introduce a debounce (I believe ng-model-options provide that out of the box).

  2. File upload functionality appears to have been broken, although this may be to do with another PR related to CSP headers - could you rebase and see if it fixes the issue?

@frankchn frankchn force-pushed the frank-search-response branch from f73df5e to 5f56ba6 Compare August 9, 2020 07:18
@frankchn
Copy link
Contributor Author

frankchn commented Aug 9, 2020

@liangyuanruo Thanks for the comments! I updated the functionality per your suggestion in (1) and tested (2) to verify that file upload works with the latest rebase.

@frankchn frankchn force-pushed the frank-search-response branch from 5f56ba6 to d79ec17 Compare August 9, 2020 07:20
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.

Just tested this branch on the staging environment - the database query isn't using existing indexes when the ObjectID is included in the query, triggering a collection scan and eventual 504 Gateway Timeout.

image

I think we'll have to investigate further on what's the best way to improve this. Some of the steps in the aggregate pipeline is clearly unnecessary if a query is only going to return metadata of a single response, so perhaps we could cater to 3 query types

  1. Valid submission ID included - return single result (TODO)
  2. Invalid submission ID - return empty output (done)
  3. No submission ID - execute existing production query

You can force push to staging branch if you'd like to test out the performance on a production-scale dataset:

git push -f origin frank-search-response:staging

@frankchn
Copy link
Contributor Author

frankchn commented Aug 9, 2020

OK I guess if MongoDB isn't quite smart enough to use the index, we will just have to do a single item query.

@frankchn
Copy link
Contributor Author

frankchn commented Aug 9, 2020

@liangyuanruo Tested on staging and both attachment upload and filtering should work now that we switched to Submission.findOne instead of using the same mechanism.

@liangyuanruo liangyuanruo merged commit eaef2eb into develop Aug 11, 2020
@liangyuanruo liangyuanruo deleted the frank-search-response branch August 11, 2020 02:25
mantariksh added a commit that referenced this pull request Aug 14, 2020
liangyuanruo added a commit that referenced this pull request Aug 15, 2020
[develop] Release 4.30.4

Revert "feat: Filter Storage Mode Responses by Submission Id (#71)"
* This reverts commit eaef2eb.
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.

3 participants