[MPDX-7852] -clear contact selection after merge #885

wjames111 · 2024-03-04T20:20:12Z

Description

Jira ticket #7852. After a successful contact merge, we need to clear the checkboxes. Currently, it keeps the checkboxes checked, causing people to merge multiple contacts if they aren't paying close attention to the prompts (It is easy to miss).

Checklist:

I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
I have requested a review from another person on the project

aws-amplify-us-east-1 · 2024-03-04T20:37:24Z

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

Access this pull request here: https://pr-885.d3dytjb8adxkk5.amplifyapp.com

canac · 2024-03-04T23:05:50Z

Instead of deselecting everything, I wonder if we should have the mass actions modal tell the context which ids were the losers of the merge and don't exist anymore, and just deselecting those contacts?

canac

The code seems to work as intented! I think you should either add the deselect all to the context (that's what I'd recommend) or prop drill but not both.

canac · 2024-03-04T23:08:52Z

src/components/Shared/MassActions/ContactsMassActionsDropdown.tsx

@@ -37,6 +38,7 @@ export const ContactsMassActionsDropdown: React.FC<
  contactsView,
  buttonGroup,
  selectedIds,
+  massDeselectAll,


Can't you get the deselect all method from <ContactsContext> instead of prop drilling?

wjames111 · 2024-03-04T23:25:59Z

@canac yeah it is a bit messy. I was trying to go with what was there already, so I didn't have to provide <ContactsContext> to another component, but the end result ends up being confusing. I'll update it to just use <ContactsContext> directly. I couldn't think of an instance where clearing all would be a bad thing but I like the idea of just deselecting the winner after merge. It's cleaner that way.

dr-bizz · 2024-03-05T13:40:33Z

@canac

Instead of deselecting everything, I wonder if we should have the mass actions modal tell the context which ids were the losers of the merge and don't exist anymore, and just deselecting those contacts?

Is this wise? The original task mentions that after doing a successful merge, some users would do another merge but since the first contact(s) are still selected, they would merge that contact too. (I had a few Helpscout tickets about this.)

I think it's best to deselect all contacts after a successful merge, if the user wants to select that contact again, they can.
Also, the only contact that will be selected after a successful merge will be the winner. massDeselectAll or toggleSelectionById I don't see the difference.

dr-bizz

Great work on this. I mentioned a few things in a comment above and one place we need to add a test. But apart from that it's looking great.

I did see when testing, that the select all checkbox still thinks something is checked after a successful merge.

dr-bizz · 2024-03-05T13:51:20Z

src/components/Shared/MassActions/ContactsMassActionsDropdown.test.tsx

+                <ContactsProvider
+                  urlFilters={{}}
+                  activeFilters={{}}
+                  setActiveFilters={() => {}}
+                  starredFilter={{}}
+                  setStarredFilter={() => {}}
+                  filterPanelOpen={false}
+                  setFilterPanelOpen={() => {}}
+                  contactId={[]}
+                  searchTerm={'test'}
+                >


Can you test that toggleSelectionById is run after a successful merge? I believe you can do it using the below.

const toggleSelectionById = jest.fn() <ContactsProvider value={{ toggleSelectionById }} >

Sure thing! thanks for the snippet, I was struggling to figure out how to use useContext in a test.

canac · 2024-03-05T15:21:57Z

Is this wise? The original task mentions that after doing a successful merge, some users would do another merge but since the first contact(s) are still selected, they would merge that contact too. (I had a few Helpscout tickets about this.)

@dr-bizz That's valid. Sorry, I didn't read the ticket super closely. My intuition is that the contact would stay selected, but I can also see how that could be confusing to some users.

@wjames111 You can disregard my original comment.

canac

It works well locally for me! Left a few style suggestions.

src/components/Contacts/MassActions/Merge/MassActionsMergeModal.test.tsx

src/components/Shared/MassActions/ContactsMassActionsDropdown.test.tsx

src/components/Contacts/MassActions/Merge/MassActionsMergeModal.test.tsx

canac · 2024-03-05T21:53:14Z

src/components/Contacts/MassActions/Merge/MassActionsMergeModal.test.tsx

 describe('ExportModal', () => {
-  it('should render modal', () => {
-    const { getByText } = render(
+  const MassActionsMergeModalWrapper: React.FC<


Can you move this outside of the describe block?

dr-bizz

Everything looks great! I left a few comments, but nothing major. You may feel like you don't need to change anything, so I approved it.

dr-bizz · 2024-03-07T15:23:31Z

src/components/Contacts/MassActions/Merge/MassActionsMergeModal.tsx

@@ -41,6 +45,8 @@ export const MassActionsMergeModal: React.FC<MassActionsMergeModalProps> = ({
  accountListId,
  ids,
 }) => {
+  const { deselectAll } = useContext(ContactsContext) as ContactsType;
+


You could remove this empty line

dr-bizz · 2024-03-07T15:25:46Z

src/components/Contacts/MassActions/Merge/MassActionsMergeModal.test.tsx

+  );
+};
+
+describe('ExportModal', () => {


Should the name of this be MassActionsMergeModal?

Yeah I think it should be, keep with the naming conventions.

wjames111 changed the title ~~7852 merge clear contacts~~ [MPDX-7852] -clear contact selection after merge Mar 4, 2024

wjames111 self-assigned this Mar 4, 2024

wjames111 added the On Staging Will be merged to the staging branch by Github Actions label Mar 4, 2024

wjames111 requested review from canac, caleballdrin and dr-bizz March 4, 2024 20:22

canac reviewed Mar 4, 2024

View reviewed changes

dr-bizz requested changes Mar 5, 2024

View reviewed changes

canac reviewed Mar 5, 2024

View reviewed changes

dr-bizz approved these changes Mar 7, 2024

View reviewed changes

Deselects all contacts on completed merge.

9055182

wjames111 force-pushed the 7852-merge-clear-contacts branch from 7805e44 to 9055182 Compare March 19, 2024 20:37

wjames111 merged commit 3c9d19c into main Mar 19, 2024
18 checks passed

wjames111 deleted the 7852-merge-clear-contacts branch March 19, 2024 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MPDX-7852] -clear contact selection after merge #885

[MPDX-7852] -clear contact selection after merge #885

wjames111 commented Mar 4, 2024 •

edited

Loading

aws-amplify-us-east-1 bot commented Mar 4, 2024

canac commented Mar 4, 2024

canac left a comment

canac Mar 4, 2024

wjames111 commented Mar 4, 2024

dr-bizz commented Mar 5, 2024 •

edited

Loading

dr-bizz left a comment

dr-bizz Mar 5, 2024

wjames111 Mar 5, 2024

canac commented Mar 5, 2024

canac left a comment

canac Mar 5, 2024

dr-bizz left a comment •

edited

Loading

dr-bizz Mar 7, 2024

dr-bizz Mar 7, 2024

wjames111 Mar 7, 2024

[MPDX-7852] -clear contact selection after merge #885

[MPDX-7852] -clear contact selection after merge #885

Conversation

wjames111 commented Mar 4, 2024 • edited Loading

Description

Checklist:

aws-amplify-us-east-1 bot commented Mar 4, 2024

canac commented Mar 4, 2024

canac left a comment

Choose a reason for hiding this comment

canac Mar 4, 2024

Choose a reason for hiding this comment

wjames111 commented Mar 4, 2024

dr-bizz commented Mar 5, 2024 • edited Loading

dr-bizz left a comment

Choose a reason for hiding this comment

dr-bizz Mar 5, 2024

Choose a reason for hiding this comment

wjames111 Mar 5, 2024

Choose a reason for hiding this comment

canac commented Mar 5, 2024

canac left a comment

Choose a reason for hiding this comment

canac Mar 5, 2024

Choose a reason for hiding this comment

dr-bizz left a comment • edited Loading

Choose a reason for hiding this comment

dr-bizz Mar 7, 2024

Choose a reason for hiding this comment

dr-bizz Mar 7, 2024

Choose a reason for hiding this comment

wjames111 Mar 7, 2024

Choose a reason for hiding this comment

wjames111 commented Mar 4, 2024 •

edited

Loading

dr-bizz commented Mar 5, 2024 •

edited

Loading

dr-bizz left a comment •

edited

Loading