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

[Publisher][Admin Panel] Add functionality to choose metrics/child agencies during metric copy #1282

Merged

Conversation

nasaownsky
Copy link
Collaborator

Description of the change

Added functionality to choose metrics/child agencies during metric copy

Type of change

All pull requests must have at least one of the following labels applied (otherwise the PR will fail):

Label Description
Type: Bug non-breaking change that fixes an issue
Type: Feature non-breaking change that adds functionality
Type: Breaking Change fix or feature that would cause existing functionality to not work as expected
Type: Non-breaking refactor change addresses some tech debt item or prepares for a later change, but does not change functionality
Type: Configuration Change adjusts configuration to achieve some end related to functionality, development, performance, or security
Type: Dependency Upgrade upgrades a project dependency - these changes are not included in release notes

Related issues

closes #1248

Checklists

Development

This box MUST be checked by the submitter prior to merging:

  • Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

  • Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

…encies by sectors (#1283)

* Add functionality to search metrics/child agencies by sectors

* Add sector to metric name
Copy link
Contributor

@mxosman mxosman left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you so much, @nasaownsky! Just a few comments - nothing major stood out to me. Will approve once I've had a chance to test this with a few agencies!

Couple of quick things:

  • Can you add a "No child agencies selected" in the chip container when user deselects all?
  • Can we remove/adjust the scroll to bottom logic now that this section has grown? It feels a bit wonky to go all the way to the bottom and scroll up - maybe scroll until the top of the copy metrics checkbox is reached?
Screen.Recording.2024-04-09.at.1.15.34.PM.mov

…to nasaownsky/1248-choose-agencies-and-metrics-during-metric-copy
@nasaownsky
Copy link
Collaborator Author

@mxosman Fixes applied, check them (and scroll behavior) out!

@mxosman
Copy link
Contributor

mxosman commented Apr 10, 2024

@mxosman Fixes applied, check them (and scroll behavior) out!

Thank you so much, @nasaownsky! It all looks great to me! We're going to hold off on merging until we can test as a group tomorrow before launching. I'll let you know as soon as this is ready to go. 😄

@mxosman
Copy link
Contributor

mxosman commented Apr 12, 2024

Hi @nasaownsky - just sharing a couple of requests from our playtesting session relevant to the FE - almost there!

  • As Lili pointed out - I think we'll need to update the call to the endpoint to include a list of selected child agencies, and pass those in.

  • Can we exclude Superagency metrics from the list of available metrics to choose from? These only belong to superagencies and cannot be copied over.
    Screenshot 2024-04-12 at 8 29 36 AM

  • Can we move the warning label to above the search boxes and make it a bit wider to reduce the height (thank you for this idea @brandon-hills)?

@mxosman
Copy link
Contributor

mxosman commented Apr 18, 2024

Thank you so much for applying to changes, @nasaownsky! One other thing that occurred to me - can we block saving if a user has checked that they want to copy metrics, but either metric selection or child agency selection is empty? (Please feel free to refactor the saving logic if need be)

@mxosman
Copy link
Contributor

mxosman commented Apr 18, 2024

Oh, by the way - there's no urgency to merge this in yet as I believe there are some pending backend changes. I'll give you a heads up as soon as things are ready to go and I/the team have had a chance to test this end to end!

@nasaownsky
Copy link
Collaborator Author

@mxosman got it!

@mxosman
Copy link
Contributor

mxosman commented Apr 19, 2024

@mxosman got it!

One small thing @nasaownsky - I just played around with it and I think there's one small edge case that lets me bypass the blocked save - if the metrics and child agencies are both deselected, deselecting all and then reselecting a sector let's me through.

@nasaownsky
Copy link
Collaborator Author

@mxosman could you please provide screen recording for that edge case? I couldn't reproduce it unfortunately.

@mxosman
Copy link
Contributor

mxosman commented Apr 23, 2024

@mxosman could you please provide screen recording for that edge case? I couldn't reproduce it unfortunately.

Absolutely! Here's a loom video below - apologies if I didn't explain this well:

https://www.loom.com/share/ffb754f5ae164f22bed66c893a425d1a

The expected behavior here should still block the saving since we have empty inputs for the child agencies and metrics.

@nasaownsky
Copy link
Collaborator Author

@mxosman should be working as expected now!

@mxosman
Copy link
Contributor

mxosman commented Apr 24, 2024

@mxosman should be working as expected now!

Thank you, @nasaownsky! I left one tiny nit - but this looks good to me! @nichelle-hall and I will test this end-to-end when the backend is ready! I appreciate all your help on this!

Copy link
Contributor

@mxosman mxosman left a comment

Choose a reason for hiding this comment

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

So @nichelle-hall and I tested this E2E and it feels like it's in really good shape! We tested with separate superagencies and confirmed that the following scenarios work:

  • Copying a subset of metrics to a subset of child agencies
  • Copying a subset of metrics to all child agencies
  • Copying all metrics to a subset of child agencies
  • Copying all metrics to all child agencies

It's currently deployed to Nichelle's playtesting URL incase anyone wants to play with it: https://nichelletest---publisher-web-b47yvyxs3q-uc.a.run.app/

@lilidworkin
Copy link
Collaborator

WOOT incredible thank you so much @mxosman and @nichelle-hall !!!!

@mxosman
Copy link
Contributor

mxosman commented May 3, 2024

Thank you, @lilidworkin! And a huge thanks to @nasaownsky for making this happen!! And huge thanks to you and @nichelle-hall for the backend work!

@nasaownsky - since you worked so hard on this, would you like the satisfaction of merging this in?

@nasaownsky
Copy link
Collaborator Author

nasaownsky commented May 3, 2024

Thank you all for the hard work, we did an amazing job! And yes please @mxosman, I would glad to :)

@nasaownsky nasaownsky merged commit 8980d24 into main May 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants