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: add filter period to iframe src for custom widgets #3183

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

EdsonNhancale
Copy link

@EdsonNhancale EdsonNhancale commented Jan 15, 2025

Key features

  1. Support for adding pe (period) as a query parameter in the iframe source URL.
  2. Dynamically appends the pe parameter to the iframe source when the filter is applied.

Description

This update ensures that the pe (period) filter is included in the iframe URL for dashboard items when the filter is applied. If the pe filter is present in itemFilters, the periods are concatenated using , as the delimiter and added as a period query parameter in the iframe source.


TODO

  • Verify the behavior with different combinations of filters (FILTER_ORG_UNIT and FILTER_PE).

TESTS

  • Added tests to validate the behavior of the period filter with single and multiple values.
  • Ensured proper handling when both pe and ou filters are applied together.
  • Included a test to confirm period filter is omitted when empty
  • Verified the generated iframeSrc matches the expected URL for all scenarios.

Known issues

  • When using custom widgets installed on the dashboard, the widget cannot currently react to changes in the period (pe) filter. This limitation arises because the pe filter value isn't dynamically communicated to the widget. A follow-up issue may be required to address this behavior.

Screenshots

403252875-57e89dd7-3737-4297-9a33-5b5ee5181e84

403267034-3fc7b907-5603-4a61-9131-48e13d97a7a9


Example:
If the itemFilters contains:

{
    FILTER_ORG_UNIT: [{ id: 'ou1', path: '/root/ou1' }, { id: 'ou2' }],
    FILTER_PE: [{id:'THIS_MONTH'}, {id:'LAST_MONTH'}]
}

Copy link
Contributor

@KaiVandivier KaiVandivier 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 opening this Edson! 🙏 The logic looks good 👍

There's some things we should do to clean up the PR before we ask analytics folks to review it:

  1. Add a more descriptive PR name, using conventional commit style, like "feat: add filter period to iframe src for custom widgets"
  2. In the description, some code block styling is wrapping the screenshots, so the images aren't visible -- move those screenshots out of the code block so they're easy to see
  3. There are some erroneous changes in en.pot and FilterSelector.js -- remove those
  4. I added some inline comments in the code -- if you use the GitHub interface to apply them, make sure to use commit messages that follow the conventional commit style (start with chore:/fix:/refactor:)

Thanks again for setting this up! Let me know if you have any questions 🙂

src/components/Item/AppItem/getIframeSrc.js Outdated Show resolved Hide resolved
src/components/Item/AppItem/getIframeSrc.js Outdated Show resolved Hide resolved
src/components/Item/AppItem/getIframeSrc.js Outdated Show resolved Hide resolved
@EdsonNhancale EdsonNhancale changed the title Iframesrc add pe feat: add filter period to iframe src for custom widgets Jan 15, 2025
EdsonNhancale and others added 8 commits January 15, 2025 09:32
Changed the delimiter for `pe` values from semicolon (`;`) to comma (`,`), aligning with the standard format for query parameters. This change improves consistency and simplifies downstream processing.

Co-authored-by: Kai Vandivier <[email protected]>
Replaced the explicit `if` condition `itemFilters[FILTER_PE] && itemFilters[FILTER_PE].length` with the optional chaining expression `itemFilters[FILTER_PE]?.length`, as recommended by SonarQube.

Co-authored-by: Kai Vandivier <[email protected]>
Updated the query parameter `pe` to `period` in the iframe source URL to improve readability and better match the naming convention of the org unit parameter.

Co-authored-by: Kai Vandivier <[email protected]>
Deleted the `i18n/en.pot` file to prevent potential conflicts with other translations or resources. This resolves issues with unnecessary file changes during development and ensures consistency in localization files.
Cleaned up the `FilterSelector.js` file by removing redundant or unnecessary changes.
Reverted unintended changes in the `i18n/en.pot`
Reverted unintended changes in the `FilterSelector.js` file
@jenniferarnesen
Copy link
Collaborator

Thank you for submitting this pull request. Could you please have a look at the unit tests in file __tests__/getIframeSrc.spec.js and add relevant tests for the new code including:

  • only pe filter (including cases with just 1 and >1 filters)
  • pe filter and ou filter

…ameSrc

- Added tests to validate the behavior of the period (pe) filter with single and multiple values. - Ensured proper handling when both pe and ou filters are applied together. - Included a test to confirm pe filter is omitted when empty. - Verified the generated iframeSrc matches the expected URL for all scenarios.
@jenniferarnesen
Copy link
Collaborator

Hi Edson,
There are lint failures. You can ignore the lint-commit error (unless you want to rebase and fix it that way) because we'll do a squash commit when the time comes and rewrite the commit message at that point. But the lint error has to be fixed.

…ameSrc

- Added tests to validate the behavior of the period (pe) filter with single and multiple values. - Ensured proper handling when both pe and ou filters are applied together. - Included a test to confirm pe filter is omitted when empty. - Verified the generated iframeSrc matches the expected URL for all scenarios.
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