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: update the summary table to display a message #2002

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

Conversation

Chisomchima
Copy link
Contributor

@Chisomchima Chisomchima commented Mar 28, 2024

Implements DHIS2-17164

Description

In the case of a dry run, this commit updates the table summary (screenshots below), and instead show a Notification that says "{ignoredCount} entities will be ignored out of {totalCount} entities"


Known issues

  • None

Checklist

  • API docs are generated
  • Tests were added
  • Storybook demos were added

All points above should be relevant for feature PRs. For bugfixes, some points might not be relevant. In that case, just check them anyway to signal the work is done.


Screenshots

Screenshot 2024-03-28 at 11 53 27 Screenshot 2024-03-28 at 15 28 56

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Mar 28, 2024

🚀 Deployed on https://pr-2002--dhis2-import-export.netlify.app

@Chisomchima Chisomchima marked this pull request as draft March 28, 2024 14:50
@Chisomchima Chisomchima marked this pull request as draft March 28, 2024 14:50
@Chisomchima Chisomchima marked this pull request as draft March 28, 2024 14:50
@Chisomchima Chisomchima force-pushed the DHIS2-17164/dryrun-summary-update branch 2 times, most recently from 1c53038 to d4a9ee7 Compare March 28, 2024 15:19
@Chisomchima Chisomchima marked this pull request as ready for review March 28, 2024 15:30
@Chisomchima Chisomchima force-pushed the DHIS2-17164/dryrun-summary-update branch 2 times, most recently from afc1b83 to e0df447 Compare March 28, 2024 16:10
@Chisomchima Chisomchima self-assigned this Mar 28, 2024
@Chisomchima Chisomchima marked this pull request as draft March 28, 2024 16:25
@Chisomchima Chisomchima force-pushed the DHIS2-17164/dryrun-summary-update branch from 2941960 to 4434f17 Compare March 28, 2024 16:30
Copy link
Contributor

@kabaros kabaros left a comment

Choose a reason for hiding this comment

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

one comment + please fix failing tests

const Summary = ({ summary, importType }) => {
// gml import type object return
const Summary = ({ summary, importType, isDryRun }) => {
if (isDryRun) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic looks like it would apply to all import dry runs, but we want to only apply it to tracked entities and events .. right?

@Chisomchima Chisomchima force-pushed the DHIS2-17164/dryrun-summary-update branch 2 times, most recently from 21bca40 to 82f372c Compare April 2, 2024 14:18
…f a table when isdryrun is true

feat: update the summary to display noticebox for pages based on import type
@Chisomchima Chisomchima force-pushed the DHIS2-17164/dryrun-summary-update branch from 82f372c to 09c2b19 Compare April 3, 2024 07:38
const Summary = ({ summary, importType }) => {
// gml import type object return
const Summary = ({ summary, importType, isDryRun }) => {
if (isDryRun && importType === 'TRACKER_IMPORT_JOB') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just put this in a variable to make the intention clearer

Suggested change
if (isDryRun && importType === 'TRACKER_IMPORT_JOB') {
const isTrackerDryRun = isDryRun && importType === 'TRACKER_IMPORT_JOB
if (isTrackerDryRun) {

and maybe add a comment for posterity:

// the new tracker endpoints don't return a full summary, so we're just showing the ignored/total

@Chisomchima Chisomchima marked this pull request as ready for review April 24, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants