-
Notifications
You must be signed in to change notification settings - Fork 258
feat(#8462): support exporting users devices details #8797
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
Conversation
cbb2b65
to
b290769
Compare
@jkuester thanks for catching the date comparison bug. I couldn't reproduce the admin UI bug you mentioned but my guess is the export is taking a looong time and there is just no feedback on the UI to reflect that so I went ahead and added some. |
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.
Very nice! Got a number of minor questions/comments that I left in-line. Regarding e2e tests, I wonder if it would be sufficient to just add a test or two to tests/integration/api/controllers/export-data.spec.js
. Seems like an integration test where we can manually create the medic-users-meta
data is probably the best approach to testing this functionality (since we don't want to wait for Sentinal).
Also, don't forget a cht-docs PR for the new API!
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.
Nice job! Josh cover a lot already, I just added some minor ones I spotted
I opened a docs PR to document this new route |
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.
LGTM! Just waiting for the translations
BTW I tried to test it but when downloading the file I got: missing_named_view
What are the steps to test it from a fresh instance?
@latin-panda I think you just need to run |
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.
We have the basic cover with the unit test added to this PR.
As suggested in the PR comment, we can evaluate e2e implementation in future iterations.
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.
LGTM!
Trying to strike a balance here between avoiding polluting the view with bad data from bad telemetry docs vs hiding the latest telemetry data from an incomplete doc. Without the user/date we cannot actually calculate the latest telemetry for the user. But, if we have that information, it seems reasonable to output whatever data we actually have from the doc (otherwise we could hide what we know is the latest data).
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.
Nice improvements! Just minor comments below
Co-authored-by: Jennifer Q <[email protected]>
@latin-panda Could I get one more quick review! The main change is a small behavior switch based on Gareth's feedback. |
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.
LGTM! Thanks Josh!
Description
#8462
Docs PR: medic/cht-docs#1290
As per #8462 (comment), this PR adds a way for admins to export user devices details to CSV. It builds on the data exporter already present in the API that comes with streaming and pagination built in.
I added the minimum amount of code in the admin app to let admins use it, they will find in the "User devices" tab of the export page.
There was some discussion about this in the issue but the new CouchDB view used to get every user's most recently used device's telemetry entry is kind of slow. It's not terrible but it's not ideal either and I don't see a faster way to do it, let me know if you have some ideas!
Side note about couchdb views DX, they shipped a new JS engine with couch 3.0 that supports ES6 so I reflected that in our eslint config.
Note: I wanted to implement an e2e test that generates telemetry entries and then export users devices from the admin app but users telemetry data gets saved to their own
medic-${username}-meta
db before getting replicated to the commonmedic-users-meta
db, where the exporter gets its data from. This replication process happens once a day at 2am and I couldn't come up with a proper way to make that happen within a testable timeframe.The closest I got to a "proper solution" is by duplicating some code from sentinel into our test to force a replication before exporting the csv.
Since this feature is planned to evolve towards a smarter way to tell admins "that user needs to upgrade this thing", we will revisit how/what we test this feature.
To do before merging:
can_export_devices_details
permission - still open to suggestions for a different permission nameCode review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.