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

fix: Exclude pending accounts from Take Now & Assign (M2-8035) #1968

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

farmerpaul
Copy link
Contributor

@farmerpaul farmerpaul commented Oct 29, 2024

  • Tests for the changes have been added
  • Delivered the fix or feature branches into develop or release branches via Squash and Merge (to keep clean history)

📝 Description

🔗 Jira Ticket M2-8035

Pending accounts shouldn't be selectable for either Take Now or Assign Activity. Updated the useParticipantDropdown hook to filter out pending accounts (by default, but it can still be overridden via a prop if ever desired).

Also disabled clicking pending participants from the Applet > Participants table (after confirming with product), to further prevent Take Now modals from being pre-populated with pending participants.

Note

✨ In making the change to Participants table row clickability, I felt it important to distinguish which rows were clickable vs. not by finally adding the table row hover effect, which was in fact always part of the original designs. To do so, I did some minor refactoring of table row hover styling, which was already present in the app on the Applets table. See this commit for details.

📸 Screenshots

Participants table row clickability

Before After
CleanShot.2024-10-31.at.09.33.04.mp4
CleanShot.2024-10-31.at.09.33.35.mp4

Take Now & Assign Activity dropdown lists

Before After
CleanShot.2024-10-31.at.09.34.39.mp4
CleanShot.2024-10-31.at.09.35.09.mp4

Improvements to Applets table & Applet Overview table row styling

Before After
CleanShot.2024-10-31.at.09.37.20.mp4
CleanShot.2024-10-31.at.09.37.49.mp4

🪤 Peer Testing

  1. Create a new full account and leave it in pending state.
  2. Perform Take Now (from anywhere in the app) and try to select that account as the subject.
    Expected outcome: The pending account is not selectable from the dropdown list.
  3. Perform Assign Activity (again, from anywhere in the app) and try to select that account as the subject of an assignment.
    Expected outcome: The pending account is not selectable from the dropdown list.
  4. Navigate to Applet > Participants and try to access the pending account's PDP by clicking its row.
    Expected outcome: The row does not respond to being clicked.

@farmerpaul farmerpaul changed the title fix: Exclude pending accounts from participant dropdowns (M2-8035) fix: Exclude pending accounts from Take Now & Assign (M2-8035) Oct 29, 2024
@farmerpaul farmerpaul added the In Progress Some work in progress label Oct 29, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1968.d19gtpld8yi51u.amplifyapp.com

Hover effects was in fact supposed to have been implemented at the very
beginning when the Participants table was added, as well as the
Applet Overview table.

Since hover effect was already in use on the main Applets table (though
using an incorrect hover colour), I went ahead and made that hover
styling reusable and aligned it with existing table row class name
convention (so that we can also enable the styling using the handy
`rowState` mechanism introduced originally for `ActivityAssignDrawer`).

Also made the `dragged-over` style definition as reusable global
styling, and improved its implementation to prevent undesirable pixel
offsets of cell content during hover.

Also applied the hover state to the Applet Overview table.
@farmerpaul farmerpaul force-pushed the fix/M2-8035-pending-accounts-showing-as-limited-accounts branch from 4791ee0 to a01f000 Compare October 31, 2024 12:40
@farmerpaul farmerpaul removed the In Progress Some work in progress label Oct 31, 2024
@farmerpaul farmerpaul marked this pull request as ready for review October 31, 2024 13:24
Copy link
Contributor

@sultanofcardio sultanofcardio left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, so I'm happy to approve. There are a few edge cases we haven't considered though, but I'm not sure if they're all worth handling in this PR so you can let me know how you want to proceed.

It is currently possible (before this change) to initiate take now with a pending account as the target subject. When the new PDP design launches, these take now submissions will show up on the by participant page of the full account that performed them. If the target subject is still a pending account, it will still be possible to initiate a take now with them pre-filled as the target subject

image image

Similarly, if we click the view data button, there is a take now button on the data viz page

image

Finally, the back button on that data viz page will take you to the PDP of the pending account.

Screen.Recording.2024-10-31.at.8.06.14.AM.mov

Do you think these should be addressed here or subsequently?

@farmerpaul farmerpaul requested review from mbanting and removed request for mbanting October 31, 2024 15:21
@farmerpaul
Copy link
Contributor Author

@sultanofcardio good point. This would have to entail that actual users have previously performed Take Now or assigned activities with a pending account as the target subject to be able to access them from the By Participant tab, right? So it feels like somewhat more of an edge-case and doesn't need to be covered by this PR.

I can already tell that it's going to either entail an awkward second request, or we need to make changes to the BE endpoint that returns the results for the expanded view, in order to discover whether an account listed there has a "pending" account status, because currently that info is not yet available in that context. So it's going to be a bigger task. I'll put it out to Cari to see if she thinks it's even worth addressing.

Copy link
Contributor

@sultanofcardio sultanofcardio left a comment

Choose a reason for hiding this comment

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

Agreed

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.

2 participants