Skip to content

chore(#9231): filter aggregate targets by period #9283

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 16 commits into from
Aug 12, 2024

Conversation

Benmuiruri
Copy link
Contributor

@Benmuiruri Benmuiruri commented Jul 24, 2024

Description

After the completion of #9267 which implements the actual filtering of the Aggregate Targets through the selection of the facilities in the radio buttons.

This PR adds the feature of filtering by reporting period. This filtering will be available for all supervisors (With one facility or with multi-facility).

Video demo and details of expected content on filtering

The first half of the video shows user with multi-facilities

  • Toggling the Health Facility on the sidebar changes the label of the LHS list from Ben's Health Facility to Second Facility
    Screenshot 2024-08-06 at 12 04 39
  • Toggling the Reporting Period on the sidebar changes the label of the LHS list from Ben's Health Facility to Ben's Health Facility - {{ Previous month }} when you select Last month
    Screenshot 2024-08-06 at 12 06 41

The second hald of the video shows user with one facility

  • When user has one facility, it does not show the facility name on the LHS and we don't show the Health Facility filter
  • When user has one facility, it shows the {{ Previous month }} on the LHS when you select Last month
Screen.Recording.2024-08-06.at.12.19.12.mov

Mobile View

Screen.Recording.2024-08-06.at.12.25.20.mov

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.

@Benmuiruri
Copy link
Contributor Author

Benmuiruri commented Jul 24, 2024

Hi @latin-panda here is an summary of what I did and what's left

What I have done

  • Display the filter button to all supervisors
  • Show facility filter options in the sidebar only to multi-facility users
  • Show reporting period filter options in the sidebar only to all users
  • Switching reporting period fetches the aggregate target by reporting period. I added this log as well to check the tag

What is left to do

  • Add translations for reporting period text
  • Add translation for reporting radio
  • change name of filter-option-place class to one more generic like filter-radio-option
  • Change the class name in sidebar-filter.less also
  • Consideration for how Facility Label breadcrumb and Reporting period period breadcrumb will display
  • Submit reports in previous months and toggle reporting period to validate correctness of data in aggregate list
  • Unit tests and possible e2e test

@latin-panda
Copy link
Contributor

@Benmuiruri I didn't have time to do much here. This is what I did:

  1. I fixed conflicts, and some things changed to reflect the default values in the indicator text.
  2. Added enums for the current and previous to not hardcode
  3. Added the selected reporting period to the indicator text - but still needs translations
Screenshot 2024-07-30 at 12 53 26 PM

@Benmuiruri
Copy link
Contributor Author

Thanks for rebasing and the progress you made, I will continue with it.

@Benmuiruri Benmuiruri marked this pull request as ready for review August 2, 2024 12:56
@Benmuiruri Benmuiruri requested a review from latin-panda August 2, 2024 13:52
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.

Exciting to see this feature almost ready for release!!

I've added some comments below

@Benmuiruri Benmuiruri requested a review from latin-panda August 5, 2024 16:48
@Benmuiruri Benmuiruri force-pushed the 9231-add-aggregate-filter branch from 0f0e074 to d0e4330 Compare August 6, 2024 06:12
@Benmuiruri Benmuiruri force-pushed the filter-aggregate-period branch from 20ac9ff to d927ee1 Compare August 6, 2024 08:32
@Benmuiruri Benmuiruri requested a review from tatilepizs August 6, 2024 11:41
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.

@Benmuiruri almost ready! I left some questions below and comments
I still need to do another round tomorrow, so I might send more feedback

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.

Thanks Ben
🤩 exciting to see this ready!! Nice job 🥳 🥳

@Benmuiruri
Copy link
Contributor Author

Hi @tatilepizs 👋 . This is waiting your review when you have some time. Thanks.

Copy link
Contributor

@tatilepizs tatilepizs left a comment

Choose a reason for hiding this comment

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

Looks really good @Benmuiruri, thank you! ⭐

@Benmuiruri Benmuiruri merged commit 1bd54df into 9231-add-aggregate-filter Aug 12, 2024
42 checks passed
@Benmuiruri Benmuiruri deleted the filter-aggregate-period branch August 12, 2024 05:53
Benmuiruri added a commit that referenced this pull request Aug 13, 2024
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