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

NAAAR analysis methods statusing: more custom logic! #12018

Open
wants to merge 1 commit into
base: naaar-am-plans
Choose a base branch
from

Conversation

gmrabian
Copy link
Contributor

@gmrabian gmrabian commented Jan 22, 2025

Description

  • Ensure statusing for analysis methods updates correctly when you remove all applicable plans
  • Tighten up display logic
    • Only show delete custom analysis method button if there are plans (meaning if they are also editable)
    • Only show custom analysis method frequency and plan text if there are plans in the plan array

Open to improvements :)

I ran into a few issues that got me here:

  1. The way we check statusing is to ensure every top level field idea is present in the data. When I changed it to included nested ids, it included the "other, specify" text, which is only required if selected, but caused the methods to fail statusing since it was in the id array. It got complicated 😓
  2. Statusing also doesn't run validation against the data, just ensures the id exists. So an empty array, empty string, etc. all counts as complete.
  3. I would have considered implementing one of the above more thoroughly, but at the moment we only have the logic for digging up all nested fields and validating them against their field types in the backend. Thus, it would have added a lot of copied code to repeat the same.

The solution I landed on does a simple check against the only offending field: plans. This is the only one that can be edited outside of the drawer (by deleting a plan on the plans page).

Related ticket(s)

CMDCT-4253


How to test

  • Create a NAAAR
  • Add three plans
  • Fill out multiple default and custom analysis methods, with various plans selected
  • See what happens as you delete all plans associated with a certain method
    • Ensure statusing changes to incomplete if there are all applicable plans were deleted
    • Ensure you cannot delete a method if all plans were deleted
    • Ensure methods where you selected "No", meaning not applicable, that it stays complete after deleting plans
    • Ensure frequency and plan text only shows up for custom methods when a plan is applicable (goes away when deleted)

Pre-review checklist

  • I have added thorough tests, if necessary
    • Please don't make me 😭
  • I have updated relevant documentation, if necessary
  • I have performed a self-review of my code
  • I have manually tested this PR in the deployed cloud environment

Pre-merge checklist

Review

  • Product: This work has been reviewed and approved by product owner, if necessary

@gmrabian gmrabian added the ready for review Ready for all the reviews! label Jan 22, 2025
@gmrabian gmrabian marked this pull request as ready for review January 22, 2025 20:57
@gmrabian gmrabian changed the title more custom logic for analysis methods and plans NAAAR analysis methods statusing: more custom logic! Jan 22, 2025
Copy link
Collaborator

@karla-vm karla-vm left a comment

Choose a reason for hiding this comment

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

works like a charm! ❇️

@@ -257,6 +257,18 @@ export const DrawerReportPage = ({ route, validateOnRender }: Props) => {
const isCustomEntity =
canAddEntities && !getDefaultAnalysisMethodIds().includes(entity.id);
const calculateEntityCompletion = () => {
// logic to ensure analysis methods always have a plan selected
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about what calculateEntityCompletion is supposed to return. An array of field ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns a boolean. The final return call ends with .every(), so these shortcuts are just returning false early.

This function is called a few lines below its definition as such

const isEntityCompleted = reportingOnIlos
        ? calculateEntityCompletion() &&
          isIlosCompleted(reportingOnIlos, entity)
        : calculateEntityCompletion();

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I would vote for changing calculateEntityCompletion() to something more boolean-sounding though isEntityCompleted is already taken...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Ready for all the reviews!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants