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

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

frankchn
Copy link
Contributor

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

Notes

This is a resubmission of #71 as the earlier PR was reverted due to performance issues.

@frankchn frankchn requested a review from liangyuanruo August 19, 2020 07:13
@liangyuanruo
Copy link
Contributor

Note - do not merge until read from primary is verified, and internal processes are complete.

@liangyuanruo liangyuanruo force-pushed the frank-filter-response branch 4 times, most recently from 881252f to c2d829a Compare September 3, 2020 04:55
@liangyuanruo
Copy link
Contributor

I think we have an issue - every time the search box is cleared, the database has to perform an expensive scan to paginate and serve the first page. This is probably the root cause of CPU saturation on secondaries.

image

@frankchn
Copy link
Contributor Author

frankchn commented Sep 4, 2020

@liangyuanruo Ah OK I didn't know the scan and serve can be so expensive on Mongo. I thought it would be more like the equivalent of a SELECT ORDER BY submission_time DESC LIMIT 10 kind of thing. Is there any way to add an index to the store to make it faster.

One thing we can do is to just add a button to trigger the filter instead of doing debouncing and triggering on keydowns.

@liangyuanruo
Copy link
Contributor

Is there any way to add an index to the store to make it faster.

We already have an index on the created column today.

One thing we can do is to just add a button to trigger the filter instead of doing debouncing and triggering on keydowns.

Something like that could work, although we should probably introduce both a "search" and a "clear" button.

I'll create a new issue for awareness since the potential fixes would involve engineering as well as design.

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.

Blocked by #276.

@frankchn frankchn force-pushed the frank-filter-response branch 2 times, most recently from 514be69 to d3fb59f Compare September 7, 2020 06:50
@frankchn
Copy link
Contributor Author

frankchn commented Sep 7, 2020

image

I have added a "Filter" button so the expensive pagination queries won't happen unless explicitly requested by the user.

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.

Can we put in a Reset button too? The current UI will confuse users because it doesn't quite make sense to filter when the search box is empty. Could we also disable the filter button if the input doesn't match the regex for a MongoDB ObjectID (/^[0-9a-fA-F]{24}$/)? This could save on network calls to the backend.

For the Reset button, I suspect we can reuse the same styling as "Open in new window".

image

@frankchn
Copy link
Contributor Author

frankchn commented Sep 7, 2020

image

I just decided to use the standard buttons instead. Probably OK for now?

@frankchn
Copy link
Contributor Author

frankchn commented Sep 7, 2020

@liangyuanruo I suspect introducing a "Clear" button that sends a request to the backend to paginate will cause the # of requests to increase.

Instead of clearing the textbox manually and pasting in the next ID, users may decide to just click the Clear button to clear the textbox. This brings us back to the same issue that we had with the original design.

@frankchn
Copy link
Contributor Author

frankchn commented Sep 8, 2020

@liangyuanruo Updated per discussion.

@frankchn frankchn force-pushed the frank-filter-response branch from 9772a76 to d88957a Compare September 14, 2020 04:48
@frankchn
Copy link
Contributor Author

@liangyuanruo I updated this based on the design in #276 -- main change being that I let the text box appear on a second row instead as it is a bit of a squeeze in the zeplin mockup if the user's screen is too narrow.

image

image

Let me know what you think!

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.

thanks @frankchn . i tested the new behavior and it's great. can we do up the UI before merging:

  1. Get the search icon to look as per the design, instead of being a blue button
  2. Address layout issues for smaller screen sizes. It might be easier to address this by implementing an input box that is positioned as per the design. The components can simply be stacked for the mobile view.
    image

@r00dgirl
Copy link
Contributor

r00dgirl commented Sep 15, 2020

@frankchn adding on

  1. replace copy 'Reset' with 'Refresh'
  2. placeholder for search box 'Search by reference no.'

@frankchn
Copy link
Contributor Author

Hi @syan-syan, made the second change, but the Reset link is actually for going back to the un-filtered view I think.

@frankchn
Copy link
Contributor Author

@liangyuanruo PTAL again, thanks!

@r00dgirl
Copy link
Contributor

Hi @syan-syan, made the second change, but the Reset link is actually for going back to the un-filtered view I think.

I know, I'm thinking it may be scary to users since 'Reset' is often associated with drastic changes (could be wiping out all the data?), while what people generally expect with 'Refresh' is what we have planned for showing the unfiltered results

@frankchn
Copy link
Contributor Author

@syan-syan No problem, updated! cc @liangyuanruo

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 @frankchn , except for a last-bit layout issue for the iPad Pro. could i trouble you to fine-tune the CSS?

image

@frankchn frankchn force-pushed the frank-filter-response branch from afb292d to 912c8b8 Compare September 21, 2020 06:56
@frankchn
Copy link
Contributor Author

image

Well, this is better but still not perfect, partly because the iPad Pro has enough horizontal resolution to be treated as a desktop browser for bootstrap purposes, so things spill out. At least all the fields are visible? :p

@frankchn
Copy link
Contributor Author

I am thinking of merging this and getting some feedback, and also changed the text for going back to looking at all responses to "View All". I think users might just end up clicking "Refresh" if they want to reload the particular filtered item.

@frankchn frankchn merged commit bcd2c61 into develop Sep 21, 2020
@frankchn frankchn deleted the frank-filter-response branch September 21, 2020 07:42
@tshuli tshuli mentioned this pull request Sep 22, 2020
tshuli pushed a commit that referenced this pull request Sep 22, 2020
* 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
mantariksh added a commit that referenced this pull request Sep 28, 2020
* feat: Filter Storage Mode Responses by Submission Id (#174)

* 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

* fix: add else on Submission.aggregate to prevent indefinite query

* refactor: fix logging format

Co-authored-by: Frank Chen <[email protected]>
Co-authored-by: Antariksh Mahajan <[email protected]>
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