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

CSY audit report: operative care #159

Open
wants to merge 10 commits into
base: feat/csy-reports-refactor
Choose a base branch
from

Conversation

deeonwuli
Copy link
Contributor

@deeonwuli deeonwuli commented Sep 19, 2024

📌 References

📝 Implementation

  • Add a new CSY report: operative care
  • Use a fixed start year for all CSY report period filters

📹 Screenshots/Screen capture

Screen.Recording.2024-09-23.at.15.03.28.mov

🔥 Notes to the tester

REACT_APP_DHIS2_BASE_URL=https://dev.eyeseetea.com/who-dev-238/
REACT_APP_REPORT_VARIANT=csy-audit-operative

#8695h711d

@deeonwuli deeonwuli marked this pull request as draft September 19, 2024 14:08
@deeonwuli deeonwuli marked this pull request as ready for review September 23, 2024 14:06
Copy link
Contributor

@eperedo eperedo left a comment

Choose a reason for hiding this comment

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

[code-review only] Thanks @deeonwuli. See my comments and let me know if you have any questions.

<div>
<Filters values={filters} options={filterOptions} onChange={setFilters} />
<p>
Audit Definition: <strong>{auditDefinition}</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use i18n instead plain text. i18n.t("Audit Definition:")


const metadata = {
programs: {
operativeCareProgramId: "Cd144iCAheH",
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this could be a lot of more code and work, but normally we prefer to use codes instead hardcoding ids. That way we can prevent future changes if id's are not the same between different environments (local, dev, production, etc).

const metadata = {
  programs: {
   operativeCareProgramCode: "PROGRAM_CODE"
  }
}

// then we can use the /metadata endpoint to get the details

api.metadata.get({})

This is something to discuss with the project manager since this change will not have any impact on the functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eperedo Miquel has suggested that we skip this at the moment. hopefully i will make the changes in a future PR


const yearItems = useMemoOptionsFromStrings(filterOptions.periods);

const quarterPeriodItems = React.useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but since we're not really doing any modification on these objects we can move them outside the component.

const quarterPeriodItems = [
            { value: "Q1", text: i18n.t("Jan - March") },
            { value: "Q2", text: i18n.t("April - June") },
            { value: "Q3", text: i18n.t("July - September") },
            { value: "Q4", text: i18n.t("October - December") },
        ]
        
   function MyComponent() {
     // use quarterPeriodItems directly
     return (
             <SingleDropdownStyled
                        items={quarterPeriodItems}
                        value={filter.quarter}
                        onChange={setQuarterPeriod}
                        label={i18n.t("Quarter")}
                        hideEmpty
                    />
     )
   }

Copy link
Contributor

@eperedo eperedo left a comment

Choose a reason for hiding this comment

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

Looks good to me. thanks @deeonwuli. Code-review only from my side since I'll need more details about the functionality to do a full test. @MiquelAdell

@MiquelAdell
Copy link
Contributor

@eperedo I passed it along to the client for testing, don't worry.
Thanks!

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