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

List users and patients #14

Merged
merged 60 commits into from
Jul 23, 2024
Merged

List users and patients #14

merged 60 commits into from
Jul 23, 2024

Conversation

arkadiuszbachorski
Copy link
Collaborator

@arkadiuszbachorski arkadiuszbachorski commented Jul 15, 2024

List users and patients

♻️ Current situation & Problem

Users and patients need to be listed.

⚙️ Release Notes

  • Add features to DataTable
  • Create "Users" list page
  • Create "Patients list" page

✅ Testing

Patients and users views will be covered with E2E tests later, hence drop in code coverage

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 46.45477% with 219 lines in your changes missing coverage. Please review.

Project coverage is 55.87%. Comparing base (ceaa0ba) to head (ac66690).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   61.47%   55.87%   -5.60%     
==========================================
  Files          73       99      +26     
  Lines         558      938     +380     
  Branches       90      169      +79     
==========================================
+ Hits          343      524     +181     
- Misses        204      390     +186     
- Partials       11       24      +13     
Files Coverage Δ
app/(dashboard)/User.tsx 0.00% <ø> (ø)
authServiceWorker.ts 0.00% <ø> (ø)
...ges/design-system/src/components/Button/Button.tsx 100.00% <ø> (ø)
...design-system/src/components/CopyText/CopyText.tsx 100.00% <100.00%> (ø)
...es/design-system/src/components/CopyText/index.tsx 100.00% <100.00%> (ø)
...system/src/components/DataTable/DataTable.mocks.ts 100.00% <100.00%> (ø)
...system/src/components/DataTable/DataTable.utils.ts 100.00% <100.00%> (ø)
...ign-system/src/components/DataTable/EmptyState.tsx 100.00% <100.00%> (ø)
...tem/src/components/DataTable/GlobalFilterInput.tsx 100.00% <100.00%> (ø)
...ents/DataTable/RowDropdownMenu/RowDropdownMenu.tsx 100.00% <100.00%> (ø)
... and 36 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ceaa0ba...ac66690. Read the comment docs.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for working on these features @arkadiuszbachorski!

I had a very quick first look!
It would be great to get the new components into the storyboard to inspect more closely but everything already looks cool on the video you shared on Slack!

I have also tried to run the current setup locally but unfortunately get the following error once logging in:
Screenshot 2024-07-15 at 5 51 52 PM

Could be that this is an issue with the backend authentication, I at least used a user with admin permission according to the Firebase datastore. Let me know what setup you use at the moment to render the components and to inspect the setup.

We should setup a Firebase-Emulator enabled testing environment to make sure that we can easily test the mobile apps & web dashboard in a reproducible manner. CC @pauljohanneskraft.

package.json Outdated
"@tanstack/react-table": "^8.19.2",
"firebase": "^10.12.2",
"firebase-admin": "^12.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

TODO to ensure that we remove the admin SDK before we merge the PR; we should ensure that we don't use the admin SDK in the client-facing web dashboard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

firebase-admin is used for retrieving user's auth data. We can't get it through Firestore. For example, to retrieve patients data, we need to combine data from user's auth and user collection.

User can access other users data only if authenticated, for their respective roles and firebase-admin is executed on the server only. I agree it probably would be better if it was separate Firebase function, but that shouldn't be blocking for now.

Copy link
Member

Choose a reason for hiding this comment

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

@pauljohanneskraft Would it make sense & doable to extract that functionality in a cloud function so we can avoid that work in the frontend codebase & ensure that we don't need to embed admin SDK keys?

On a general level: I think we should generally use the firebase queries using the client SDK when possible to ensure that we build on top of the Security Rules but in some edge cases we should selectively use cloud functions if needed.

modules/firebase/utils.ts Outdated Show resolved Hide resolved
modules/firebase/utils.ts Show resolved Hide resolved
@arkadiuszbachorski
Copy link
Collaborator Author

I had a very quick first look! It would be great to get the new components into the storyboard to inspect more closely but everything already looks cool on the video you shared on Slack!

Everything will have their Storybooks, WIP!

Could be that this is an issue with the backend authentication, I at least used a user with admin permission according to the Firebase datastore. Let me know what setup you use at the moment to render the components and to inspect the setup.

This was caused by wrong permission rules, the thing I raised on Slack. I just rollbacked to previous rules, it should work correctly now.

We should setup a Firebase-Emulator enabled testing environment to make sure that we can easily test the mobile apps & web dashboard in a reproducible manner. CC @pauljohanneskraft.

Agree, now I am just using live Firebase data.

@arkadiuszbachorski arkadiuszbachorski force-pushed the arek/list-users branch 2 times, most recently from 134e034 to 863971a Compare July 16, 2024 13:45
@PSchmiedmayer
Copy link
Member

Thank you for continuing to improve this @arkadiuszbachorski! Sounds good; let's sync up with @pauljohanneskraft so we can combine this with the firebase setup to have a nice emulator testing environment.

@arkadiuszbachorski arkadiuszbachorski marked this pull request as ready for review July 20, 2024 13:31
@arkadiuszbachorski
Copy link
Collaborator Author

arkadiuszbachorski commented Jul 21, 2024

@PSchmiedmayer I updated this PR with:

  • tests and storybooks
  • emulators support
  • removed usage of firebase-admin in favor of calling getUsersInformation function
  • resolved couple other todos, improvements

There is drop in coverage, because I didn't cover users and patients listing flow. It will be a separate E2E effort.

Functions types are copied from Firebase repository temporarily, it will be a shared package in the future

Base automatically changed from arek/add-dashboard to main July 23, 2024 18:36
@arkadiuszbachorski arkadiuszbachorski merged commit 720a0df into main Jul 23, 2024
8 of 10 checks passed
@arkadiuszbachorski arkadiuszbachorski deleted the arek/list-users branch July 23, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants