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) O3-1494: Add report admin dashboard #45

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

pwargulak
Copy link
Contributor

Summary

The PR contains implementation of Report administration pages, including:

  1. Reports overview page which shows history of Report executions, inc current and planned execution.
  2. Reports schedule where user can schedule reports to be execute once at some time later, daily, weekly at specific day(-s), monthly at the end or start of a month (this is a copy o OMRS 1.x functionality).

Pages are accessible via System Administration menu.
It implements designs documented here (newest mocks section).

Screenshots

screencapture-localhost-8081-openmrs-spa-reports-2024-02-16-15_34_13
screencapture-localhost-8081-openmrs-spa-reports-2024-02-16-15_34_20
screencapture-localhost-8081-openmrs-spa-reports-scheduled-overview-2024-02-16-15_34_34
screencapture-localhost-8081-openmrs-spa-reports-scheduled-overview-2024-02-16-15_35_14

Issue

https://openmrs.atlassian.net/browse/O3-1494

@gracepotma
Copy link

I expect that Palladium-Kenya will be interested in using this so I reached out to @ojwanganto: @jecihjoy he recommended you review this PR :)

@pwargulak pwargulak marked this pull request as draft February 27, 2024 15:02
@pwargulak
Copy link
Contributor Author

Changed to DRAFT, because it's not adapted to changes in API in openmrs/openmrs-module-reportingrest#33

@gracepotma gracepotma requested review from ibacher and removed request for ibacher February 27, 2024 18:39
@gracepotma
Copy link

So TBH I didn't notice until now that this PR is almost 12k lines of code - woah. Reviewing this make take our support & community members quite some time. In future smaller PRs earlier would be more advisable :) Hope that makes sense!

@ojwanganto
Copy link

@gracepotma - Should we have this work as part of the admin tools or a separate reporting-esm?

@ibacher
Copy link
Member

ibacher commented Mar 28, 2024

@ojwanganto Well, it is a separate esm, just in this mono-repo. I don't think we need individual GitHub repos for everything.

@ojwanganto
Copy link

Makes sense @ibacher. I think i didn't get into the source code. I just looked at the project URL. Sorry about that

@ojwanganto
Copy link

@pwargulak when do you think we can start testing this work? It already provides a good foundation for the reporting functionality we need

@pwargulak
Copy link
Contributor Author

@ojwanganto We are working on bringing the BE code to sufficient quality, the PRs related to BE are listed here once they are merged we'll adapt the FE (this PR) and then it can be tested.

@ojwanganto
Copy link

Thanks for the update @pwargulak.

@tendomart
Copy link

Iam curios to know if anyone has used this.. may need it somewhere @pwargulak @jecihjoy

@pwargulak pwargulak marked this pull request as ready for review January 17, 2025 12:20
@druchniewicz
Copy link
Contributor

@denniskigen @ibacher as two required backend PRs have been merged I adapted this PR to mentioned backend changes and open PR once again for review. Can you take a look? Piotr's first comment gives a quick overview what has been done here

@druchniewicz
Copy link
Contributor

@ibacher @denniskigen @jecihjoy I kindly remind you about this PR. Can you take a look?

@ojwanganto
Copy link

Looping in @makombe @donaldkibet

@druchniewicz
Copy link
Contributor

@denniskigen @ibacher @brandones @jecihjoy @makombe @donaldkibet
can you take a look at pull request? I would like to make this PR mergable to allow people using it

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Thanks for getting this off the ground, @pwargulak! I've left some suggestions.

denniskigen

This comment was marked as duplicate.

denniskigen

This comment was marked as duplicate.

@druchniewicz
Copy link
Contributor

@denniskigen I addressed your comments and also make similar corrections in other places in app, not only with those indicated by you. Can you take a look?

@druchniewicz
Copy link
Contributor

Hey @denniskigen , will you be able to look on my latest changes? (made according to your last comments)

@druchniewicz
Copy link
Contributor

@denniskigen

@druchniewicz
Copy link
Contributor

Hi @denniskigen , next changes have been pushed. Can you take a look?

@denniskigen denniskigen changed the title O3-1494: Added Report Admin pages (feat) O3-1494: Add report admin dashboard Feb 6, 2025
@denniskigen
Copy link
Member

denniskigen commented Feb 6, 2025

Great work getting this over the line, @druchniewicz @pwargulak! Here's some additional changes to consider as potential future iterations:

  1. Migrating from the custom overlay implementation in favor of using the Workspace system. Read more about the Workspace API here.
  2. Add empty states to the widgets in the UI for when there is no data to show to the user.
  3. Migrate form logic to React Hook Form and Zod for reduced boilerplate, type-safe validation and improved performance.
  4. Migrate from using breadcrumbs as a navigational pattern. @ciaranduffy @paulsonder, could you advise on what the ideal navigational pattern for pages like these should be?

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Thanks for the great work! Merging shortly; the e2e test failure seems unrelated to your changes.

@denniskigen denniskigen merged commit 626b777 into openmrs:main Feb 6, 2025
5 of 6 checks passed
@druchniewicz
Copy link
Contributor

Thanks for the great work! Merging shortly; the e2e test failure seems unrelated to your changes.

Thanks @denniskigen for reliable code review and suggestions how your comments should be addressed!

@denniskigen
Copy link
Member

Another important next step I forgot to mention: adding the reports app to the reference application's spa-assemble-config.json file so it gets included in the importmap.

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.

7 participants