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

[TTAHUB-3281] manual mark goals similar #2303

Merged
merged 50 commits into from
Aug 9, 2024

Conversation

GarrettEHill
Copy link
Collaborator

@GarrettEHill GarrettEHill commented Aug 7, 2024

Description of change

Implemented the ability to manually mark goals as similar within the Head Start TTA Hub. This new feature allows users to select multiple goals and group them together if they are determined to be similar. The change includes updates to the frontend UI to support this functionality, new routes, and backend modifications to process and store the similarity groupings.

How to test

  1. Navigate to the goals management section of the application.
  2. Select multiple goals to mark them as similar.
  3. Verify that the selected goals are grouped correctly.
  4. Ensure the backend processes and stores these groupings as expected.
  5. Run the updated unit tests to confirm all are passing.

Issue(s)

Checklists

Every PR

  • Meets issue criteria
  • JIRA ticket status updated
  • Code is meaningfully tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • API Documentation updated
  • Boundary diagram updated
  • Logical Data Model updated
  • Architectural Decision Records written for major infrastructure decisions
  • UI review complete

Before merge to main

  • OHS demo complete
  • Ready to create production PR

Production Deploy

  • Staging smoke test completed

After merge/deploy

  • Update JIRA ticket status

src/routes/recipient/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@nvms nvms left a comment

Choose a reason for hiding this comment

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

lgtm, but let's get another approval before merging

}

checkFeatureFlags();
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is an empty [] on use effect allowed by our linter rules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no complaints by the linter

Copy link
Collaborator

Choose a reason for hiding this comment

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

useEffect with empty deps is standard, and means "run only once" - what's the concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

NVM seems like its a one and done thing.


await markSimilarGoals(recipientId, similarGoals); // PUT request to mark similar goals
selectAllGoalCheckboxSelect({ target: { checked: false } }); // Deselect all goals
history.go(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we trigger a manual reload of the page?

Copy link
Collaborator Author

@GarrettEHill GarrettEHill Aug 9, 2024

Choose a reason for hiding this comment

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

so the newly added similarity group shows up in the banner

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be using state to trigger re render?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I dont know what you mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you have an alternative, that would work better, please suggest the code change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you reloading the page to display something that was not previously there? If yes its means you are changing the state of the page. In this case I would think you would want to use some sort of useState() var to trigger the re render and display the state change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pushed suggested change.

@@ -256,7 +257,7 @@ export async function createSimilarityGroup(
);
}

if (group) {
if (!byPassChecks && group) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good assuming we are cleaning up any groups that might already have these goals.

Copy link
Collaborator

@AdamAdHocTeam AdamAdHocTeam 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!

@GarrettEHill GarrettEHill merged commit 0fe2c82 into main Aug 9, 2024
10 checks passed
@GarrettEHill GarrettEHill deleted the TTAHUB-3281/manual-mark-goals-similar branch August 9, 2024 21:11
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