Skip to content

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

Merged
merged 44 commits into from
Feb 23, 2024

Conversation

m5r
Copy link
Contributor

@m5r m5r commented Jan 10, 2024

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 common medic-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:

  • [] make sonar accept the "5.3% duplication on new code" it caught, it's only admin-related code for the new tab
  • [] add a new can_export_devices_details permission - still open to suggestions for a different permission name

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

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.

@m5r m5r force-pushed the 8462-export-user-devices-details-to-csv branch from cbb2b65 to b290769 Compare January 17, 2024 13:50
@m5r
Copy link
Contributor Author

m5r commented Feb 7, 2024

@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.
We already had a property in the export template to give a "loading" feedback but the value it relied on was never set.

@m5r m5r marked this pull request as ready for review February 7, 2024 14:12
Copy link
Contributor

@jkuester jkuester left a 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!

Copy link
Contributor

@latin-panda latin-panda left a 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

@m5r
Copy link
Contributor Author

m5r commented Feb 13, 2024

I opened a docs PR to document this new route

@m5r m5r requested review from jkuester and latin-panda February 13, 2024 09:39
Copy link
Contributor

@latin-panda latin-panda left a 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?

@m5r
Copy link
Contributor Author

m5r commented Feb 13, 2024

@latin-panda I think you just need to run npm run build-ddocs to push the view to couch

@m5r m5r requested a review from latin-panda February 13, 2024 15:50
Copy link
Contributor

@lorerod lorerod left a 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.

Copy link
Contributor

@latin-panda latin-panda left a 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).
@jkuester jkuester requested a review from latin-panda February 20, 2024 22:07
Copy link
Contributor

@latin-panda latin-panda left a 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

@jkuester
Copy link
Contributor

@latin-panda Could I get one more quick review! The main change is a small behavior switch based on Gareth's feedback.

@latin-panda latin-panda linked an issue Feb 22, 2024 that may be closed by this pull request
@latin-panda latin-panda self-requested a review February 22, 2024 04:15
Copy link
Contributor

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Josh!

@jkuester jkuester changed the title feat(#8462): Export users devices details to JSON feat(#8462): support exporting users devices details Feb 23, 2024
@jkuester jkuester merged commit bdbb6ba into master Feb 23, 2024
@jkuester jkuester deleted the 8462-export-user-devices-details-to-csv branch February 23, 2024 16:52
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.

Expose user device details to admins
4 participants