feat: adding cancel and remind actions to the budget assignment table #1070

alex-sheehan-edx · 2023-10-26T20:59:19Z

Assigned selected:

Assigned and not assigned selected:

Not assigned selected:

https://2u-internal.atlassian.net/browse/ENT-7813

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

codecov · 2023-10-26T21:23:47Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1224e74) 84.14% compared to head (d77fd10) 84.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1070      +/-   ##
==========================================
+ Coverage   84.14%   84.15%   +0.01%     
==========================================
  Files         455      458       +3     
  Lines        9560     9581      +21     
  Branches     1992     1996       +4     
==========================================
+ Hits         8044     8063      +19     
- Misses       1474     1476       +2     
  Partials       42       42

Files	Coverage Δ
...earner-credit-management/AssignmentTableCancel.jsx	`100.00% <100.00%> (ø)`
...arner-credit-management/BudgetAssignmentsTable.jsx	`100.00% <ø> (ø)`
...earner-credit-management/AssignmentTableRemind.jsx	`88.88% <88.88%> (ø)`
...credit-management/AssignmentRowActionTableCell.jsx	`75.00% <75.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/components/learner-credit-management/BudgetAssignmentsTable.jsx

src/components/learner-credit-management/AssignmentRowActionTableCell.jsx

src/components/learner-credit-management/BudgetAssignmentsTable.jsx

src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx

adamstankiewicz · 2023-10-30T18:11:48Z

src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx

@@ -701,4 +701,69 @@ describe('<BudgetDetailPage />', () => {

    expect(screen.getByText('loading budget activity overview')).toBeInTheDocument();
  });
+
+  it('displays bulk actions with remind when row selected and state allocated', () => {


[clarification] What is the intent of this test case? Based on the title, I would expect this test to trigger a row to be selected, and assert that the bulk action CTAs to remind / cancel appear only after selection.

As is, though, I believe this test is asserting that the row-level remind/cancel IconButtons and associated tooltips are visible in the table row, which is not quite related to bulk actions as the test case's title would indicate.

If you're trying to test the bulk actions themselves in a test case(s), you'd need to trigger a user event to select a row.

It's about the hiding and showing of the remind action. I've updated the wording, let me know if it makes works for you

adamstankiewicz · 2023-10-30T18:21:17Z

src/components/learner-credit-management/BudgetAssignmentsTable.jsx

+const RemindAction = ({ selectedFlatRows, ...rest }) => (
+  // eslint-disable-next-line no-console
+  <Button onClick={() => console.log('Remind', selectedFlatRows, rest)}>
+    {`Remind (${selectedFlatRows.length})`}


[clarification] Should we be assuming all selected rows can be reminded? For example, if an admin selects a row with the "Status" column depicting "Notifying learner", "Failed: Bad email", etc., I'm figuring we wouldn't want to allow reminders on those rows.

Related, I see these annotations in the Figma:

Is this something that intends to be addressed as part of this PR or are there plans around the conditional logic for these bulk actions as follow-up work?

you're totally right in that we should not allow the reminding of assignments that aren't pending, however current UX is to hide the button, not disable

I've added the behavior according to the AC on the ticket

Just a heads up, this behavior as implemented may need to be revisited after hearing from Mike Brown from UX about the intended bulk action behavior.

The Figma mocks above allude that the "Assigned" table bulk actions should largely have parity with subscription management's bulk actions. However, the behavior as spec'd/implemented (hiding the bulk action when non-actionable rows are selected) is not parity with subscription bulk actions.

There are possible usability concerns with hiding the bulk action (e.g., possible lack of discoverability of the bulk action, forcing admins to manually unselect one or more rows for the bulk action to re-appear). These were mitigated in subscriptions bulk actions by keeping the bulk action visible but exclude non-actionable rows. If there are no actionable rows, the bulk action becomes disabled.

Mike/UX is revisiting this interaction now. We may need to update this behavior in code once a final decision is made.

src/components/learner-credit-management/BudgetAssignmentsTable.jsx

src/components/learner-credit-management/AssignmentRowActionTableCell.jsx

adamstankiewicz · 2023-10-31T15:42:15Z

src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx

@@ -701,4 +701,117 @@ describe('<BudgetDetailPage />', () => {

    expect(screen.getByText('loading budget activity overview')).toBeInTheDocument();
  });
+
+  it('displays remind row and bulk actions when allocated', async () => {


nit: it also asserts that the row-level Cancel CTA is there as well.

adamstankiewicz · 2023-10-31T15:44:50Z

src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx

+      expect(screen.getByText('Remind (1)')).toBeInTheDocument();
+    });
+    await waitFor(() => {
+      expect(screen.getByText('Cancel (1)')).toBeInTheDocument();


nit: likely could group both of these assertions under the single waitFor. Alternatively, could also rely on await screen.findByText('Cancel (1)') as well:

expect(await screen.findByText('Remind (1)')).toBeInTheDocument(); expect(await screen.findByText('Cancel (1)')).toBeInTheDocument();

adamstankiewicz · 2023-10-31T15:46:15Z

src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx

+    expect(screen.queryByTestId('remind-assignment-test-uuid')).not.toBeInTheDocument();
+    const checkBox = screen.getByTestId('datatable-select-column-checkbox-cell');
+    userEvent.click(checkBox);
+    expect(screen.queryByText('Remind (1)')).not.toBeInTheDocument();


[clarification] Will the "Cancel" CTA also not show in this test case? Might be good to be explicit about whether the "Cancel" CTA is present or not here as well.

alex-sheehan-edx requested a review from adamstankiewicz October 26, 2023 20:59

alex-sheehan-edx marked this pull request as draft October 26, 2023 20:59

kiram15 reviewed Oct 26, 2023

View reviewed changes

src/components/learner-credit-management/BudgetAssignmentsTable.jsx Outdated Show resolved Hide resolved

kiram15 reviewed Oct 26, 2023

View reviewed changes

src/components/learner-credit-management/BudgetAssignmentsTable.jsx Outdated Show resolved Hide resolved

adamstankiewicz reviewed Oct 30, 2023

View reviewed changes

alex-sheehan-edx force-pushed the asheehan-edx/ENT-7813-cancel-remind-actions branch from 113805d to 6cf5b62 Compare October 30, 2023 17:19

alex-sheehan-edx marked this pull request as ready for review October 30, 2023 17:25

alex-sheehan-edx force-pushed the asheehan-edx/ENT-7813-cancel-remind-actions branch 2 times, most recently from a43d361 to 6571bc7 Compare October 30, 2023 17:54

alex-sheehan-edx requested review from adamstankiewicz and kiram15 October 30, 2023 17:57

adamstankiewicz reviewed Oct 30, 2023

View reviewed changes

src/components/learner-credit-management/AssignmentRowActionTableCell.jsx Outdated Show resolved Hide resolved

src/components/learner-credit-management/AssignmentRowActionTableCell.jsx Outdated Show resolved Hide resolved

alex-sheehan-edx force-pushed the asheehan-edx/ENT-7813-cancel-remind-actions branch 5 times, most recently from 9d7fd32 to b7d9aa0 Compare October 30, 2023 21:44

feat: adding cancel and remind actions to the budget assignment table

c5bea6d

alex-sheehan-edx force-pushed the asheehan-edx/ENT-7813-cancel-remind-actions branch from b7d9aa0 to c5bea6d Compare October 31, 2023 15:56

kiram15 and others added 2 commits November 1, 2023 06:46

Merge branch 'master' into asheehan-edx/ENT-7813-cancel-remind-actions

1f99518

Merge branch 'master' into asheehan-edx/ENT-7813-cancel-remind-actions

d77fd10

kiram15 approved these changes Nov 1, 2023

View reviewed changes

kiram15 merged commit 6da44f2 into master Nov 1, 2023
5 checks passed

kiram15 deleted the asheehan-edx/ENT-7813-cancel-remind-actions branch November 1, 2023 14:36

adamstankiewicz reviewed Nov 1, 2023

View reviewed changes

kiram15 mentioned this pull request Nov 3, 2023

fix: altering bulk enrollment action behaviour #1081

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding cancel and remind actions to the budget assignment table #1070

feat: adding cancel and remind actions to the budget assignment table #1070

alex-sheehan-edx commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

adamstankiewicz Oct 30, 2023

alex-sheehan-edx Oct 30, 2023

adamstankiewicz Oct 30, 2023 •

edited

Loading

alex-sheehan-edx Oct 30, 2023

alex-sheehan-edx Oct 30, 2023

adamstankiewicz Nov 1, 2023

adamstankiewicz Oct 31, 2023

adamstankiewicz Oct 31, 2023

adamstankiewicz Oct 31, 2023

feat: adding cancel and remind actions to the budget assignment table #1070

feat: adding cancel and remind actions to the budget assignment table #1070

Conversation

alex-sheehan-edx commented Oct 26, 2023 • edited Loading

For all changes

Only if submitting a visual change

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-sheehan-edx commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

adamstankiewicz Oct 30, 2023 •

edited

Loading