Add ability to expire Entitlements #83

Ali-D-Akbar · 2021-01-21T14:08:55Z

Add ability to expire entitlements in support tool

An Expire button is added in the action column of Entitlements as shown in the image:

A form will be shown upon clicking the Expire button. The user can add a comment about the reason to expire the entitlement.

Submitting this will send a patch request to api to set expiredAt field. Note that the button will be disabled once the entitlement is expired.

codecov · 2021-01-21T14:31:59Z

Codecov Report

Merging #83 (bad2e8e) into master (f40697d) will decrease coverage by 0.02%.
The diff coverage is 18.57%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   21.86%   21.83%   -0.03%     
==========================================
  Files          28       34       +6     
  Lines         709      751      +42     
  Branches      152      155       +3     
==========================================
+ Hits          155      164       +9     
- Misses        539      572      +33     
  Partials       15       15

Impacted Files	Coverage Δ
src/users/UserPage.jsx	`62.16% <ø> (ø)`
src/users/data/api.js	`26.80% <ø> (ø)`
src/users/entitlements/Entitlements.jsx	`2.81% <0.00%> (ø)`
src/users/entitlements/utils.js	`0.00% <0.00%> (ø)`
src/users/entitlements/ExpireEntitlementForm.jsx	`11.11% <11.11%> (ø)`
src/users/entitlements/ReissueEntitlementForm.jsx	`11.76% <11.76%> (ø)`
src/users/entitlements/EntitlementForm.jsx	`25.00% <25.00%> (ø)`
src/users/entitlements/CreateEntitlementForm.jsx	`11.76% <33.33%> (ø)`
src/users/entitlements/EntitlementActions.jsx	`100.00% <100.00%> (ø)`
src/users/entitlements/PropTypes.jsx	`100.00% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f40697d...2fe77b5. Read the comment docs.

saadyousafarbi

Before review had some questions regarding the AC of the ticket being shared:

Record the name of the support person expiring the entitlement, the time of expiration, and the reason for expiration.
Is this being captured anywhere on this new flow?
If there are two entitlements appearing in a course for a user, expiring one shouldn’t impact the other in any way.
Have you tested this scenario, with learners having two entitlements for the same course? Looks like it shouldnt be a problem... But just to make sure!

Ali-D-Akbar · 2021-01-22T10:24:31Z

Record the name of the support person expiring the entitlement, the time of expiration, and the reason for expiration.
Is this being captured anywhere on this new flow?

The API handles this very purpose as you can see below.

If there are two entitlements appearing in a course for a user, expiring one shouldn’t impact the other in any way.
Have you tested this scenario, with learners having two entitlements for the same course? Looks like it shouldnt be a problem... But just to make sure!

We're making a patch request to the API with the uuid of the entitlement instead of course_uuid. This will only expire the specific entitlement.

awaisdar001

need some improvements here.

awaisdar001 · 2021-01-25T15:14:59Z

src/users/data/api.js

-            comments,
-          },
-        ],
+        expired_at: ((action === EXPIRE) ? now : null),


This API layer shouldn't construct data -- and it should use the data. so, please if you are passing this request data it's better (for clarity) to pass from the calling function.

In the future if we want to reuse patchEntitlement method we would not need to come back here and add another if condition.

awaisdar001 · 2021-01-25T15:15:15Z

src/users/entitlements/EntitlementActions.jsx

@@ -0,0 +1,3 @@
+export const REISSUE = 'reissue';


awaisdar001 · 2021-01-25T15:16:36Z

src/users/entitlements/EntitlementCreateForm.jsx

-
-export default function EntitlementForm({
-  formType,
+export default function EntitlementCreateForm({


Better => CreateEntitlementForm

awaisdar001 · 2021-01-25T15:17:11Z

src/users/entitlements/EntitlementCreateForm.jsx

  });

-  const isReissue = formType === REISSUE;
-  const title = isReissue ? 'Re-issue Entitlement' : 'Create Entitlement';
+  let title = 'Create Entitlement';


why let than const here?

awaisdar001 · 2021-01-25T15:19:20Z

src/users/entitlements/EntitlementExpireForm.jsx

+    clear('entitlements');
+    patchEntitlement({
+      uuid: entitlement.uuid,
+      action: EXPIRE,


Here you should pass the data you want to patch, don't expose app logic to the API layer.

awaisdar001 · 2021-01-25T15:22:25Z

src/users/entitlements/EntitlementForm.jsx

+}) {
+  const [courseUuid, setCourseUuid] = useState(entitlement.courseUuid);
+  const [mode, setMode] = useState(entitlement.mode);
+  const [comments, setComments] = useState('');


comments => comment

awaisdar001 · 2021-01-25T15:27:11Z

src/users/entitlements/EntitlementForm.jsx

+          forwardedRef={forwardedRef}
+        />
+      );
+    default:


Let's use if/else here. switch/case is preferred if the number of cases are more than 5 AFAIK.

src/users/entitlements/EntitlementForm.jsx

awaisdar001 · 2021-01-25T15:28:30Z

src/users/entitlements/EntitlementForm.jsx

+};
+
+EntitlementForm.defaultProps = {
+  entitlement: {


same goes here.

awaisdar001 · 2021-01-25T15:28:46Z

src/users/entitlements/EntitlementReissueForm.jsx

+    clear('entitlements');
+    patchEntitlement({
+      uuid: entitlement.uuid,
+      action: REISSUE,


same comments here.

What must be updated here? I've updated everything else except this.

awaisdar001 · 2021-01-26T14:44:41Z

src/users/entitlements/EntitlementForm.jsx

@@ -25,8 +26,7 @@ export default function EntitlementForm({
        forwardedRef={forwardedRef}
      />
    );
-  }
-  else if (formType === EXPIRE) {
+  } if (formType === EXPIRE) {


Only one form is rendering at one time so what do you think about using if/else?

Had to change it because of eslint error no-else-return
(It's weird how eslint reacts to 3 cases of if/else.)

awaisdar001 · 2021-01-26T14:46:55Z

src/users/data/api.js

@@ -225,19 +225,18 @@ export async function patchEntitlement({
  action,
  unenrolledRun = null,
  comments = null,
+  expiredAt = null,
 }) {
  try {
    const { data } = await getAuthenticatedHttpClient().patch(
      `${getConfig().LMS_BASE_URL}/api/entitlements/v1/entitlements/${uuid}/`,
      {


My understanding was when I commented previously, is to struct the following data from the calling function so API layer.

{ expired_at: expiredAt, support_details: [{ unenrolled_run: unenrolledRun, action, comments, }], },

awaisdar001 · 2021-01-26T14:51:23Z

src/users/entitlements/CreateEntitlementForm.jsx

-
-  const isReissue = formType === REISSUE;
-  const title = isReissue ? 'Re-issue Entitlement' : 'Create Entitlement';
+  const title = 'Create Entitlement';


Any reason for having this variable in place? is being reused anywhere? If it's only used in one place then this variable is not useful.

You're right. I'm removing the variable and use the text directly.

awaisdar001 · 2021-01-26T14:52:56Z

src/users/entitlements/EntitlementForm.jsx

@@ -0,0 +1,66 @@
+import React from 'react';


the name of this file can be improved to RenderForm.jsx

awaisdar001 · 2021-01-26T14:55:07Z

src/users/entitlements/EntitlementPropTypes.jsx

@@ -0,0 +1,31 @@
+import PropTypes from 'prop-types';


src/users/entitlements/PropTypes.jsx

awaisdar001 · 2021-01-26T14:59:31Z

src/users/entitlements/Entitlements.jsx

+              disabled={!result.enrollmentCourseRun}
+              onClick={() => {
+                clearCourseSummary();
+                setUserEntitlement(result);


result => entitlement

src/users/entitlements/ExpireEntitlementForm.jsx

awaisdar001 · 2021-01-27T14:32:29Z

src/users/data/api.js

@@ -221,24 +221,11 @@ export async function getCourseData(courseUUID) {
 }

 export async function patchEntitlement({


this API call is alot cleaner now. 👍
have you updated all patchEntitlement calls in the code?

Yes, I've changed the calls accordingly and tested it on the local.

awaisdar001 · 2021-01-27T14:33:37Z

src/users/entitlements/Entitlements.jsx

@@ -65,76 +66,90 @@ export default function Entitlements({
    if (data === null) {
      return [];
    }
-    return data.results.map(result => ({
+    return data.results.map(entitlement => ({


more readable with this change 👍

awaisdar001

the changes look good to me 👍

DawoudSheraz

Added a few comments.

DawoudSheraz · 2021-01-29T14:40:09Z

src/users/entitlements/RenderForm.jsx

+import ExpireEntitlementForm from './ExpireEntitlementForm';
+import { EntitlementPropTypes, EntitlementDefaultProps } from './PropTypes';
+
+export default function RenderForm({


I think the appropriate name would be EntitlementForm. RenderForm doesn't signify what is being rendered.

I originally had it named as EntitlementForm but was asked to change it by @awaisdar001

Hmm, I still think EntitlementForm is a better name. It signifies the form is for entitlement. RenderForm is a generic term because it could be to render anything.

DawoudSheraz · 2021-01-29T14:44:28Z

src/users/entitlements/ReissueEntitlementForm.jsx

+    patchEntitlement({
+      uuid: entitlement.uuid,
+      requestData: {
+        support_details: [{
+          unenrolled_run: entitlement.enrollmentCourseRun,
+          action: REISSUE,
+          comments,
+        }],
+      },


I think we can write a utility function that will return the dict representation of the data that API needs. Right now, the representation is being duplicated whenever we need to call API. Although this will not go to the API function itself, having a utility function means that in the future, if the API data signature changes, we will need to update it in fewer places.

@Ali-D-Akbar

DawoudSheraz · 2021-01-29T14:45:45Z

src/users/entitlements/Entitlements.jsx

+            <Button
+              type="button"
+              variant="outline-danger"
+              disabled={entitlement.expiredAt}


Isn't expiredAt a datetime str?

Yes, if expiredAtHAS a value other than null then the field will be disabled.

DawoudSheraz

nit comments, otherwise it is good to approve.

DawoudSheraz · 2021-02-01T14:04:39Z

src/users/entitlements/ExpireEntitlementForm.jsx

+            Submit
+          </Button>
+          <Button
+            className="btn-outline-secondary"


use variant here. It might break with paragon 12. Not a blocker but do change it in a followup PR

DawoudSheraz · 2021-02-01T14:05:06Z

src/users/entitlements/ReissueEntitlementForm.jsx

+            Submit
+          </Button>
+          <Button
+            className="btn-outline-secondary"


DawoudSheraz · 2021-02-01T14:05:41Z

src/users/entitlements/makeRequestData.js

@@ -0,0 +1,12 @@
+export default function makeRequestData({


filename should be utils.js, not makeRequestData

Ali-D-Akbar requested review from DawoudSheraz, staubina and saadyousafarbi January 21, 2021 14:34

Ali-D-Akbar changed the title ~~add ability to expire entitlements~~ Add ability to expire Entitlements Jan 21, 2021

saadyousafarbi reviewed Jan 22, 2021

View reviewed changes

Ali-D-Akbar requested a review from awaisdar001 January 25, 2021 07:40

awaisdar001 reviewed Jan 25, 2021

View reviewed changes

awaisdar001 reviewed Jan 26, 2021

View reviewed changes

Ali-D-Akbar force-pushed the aakbar/PROD-2132 branch from 4d72748 to fb67906 Compare January 27, 2021 07:01

Ali-D-Akbar requested a review from awaisdar001 January 27, 2021 07:04

Ali-D-Akbar force-pushed the aakbar/PROD-2132 branch 2 times, most recently from a41d3cd to 397cb5b Compare January 27, 2021 07:18

awaisdar001 reviewed Jan 27, 2021

View reviewed changes

awaisdar001 approved these changes Jan 27, 2021

View reviewed changes

Ali-D-Akbar force-pushed the aakbar/PROD-2132 branch 2 times, most recently from a48330a to 96b3e15 Compare January 29, 2021 12:29

DawoudSheraz reviewed Jan 29, 2021

View reviewed changes

DawoudSheraz reviewed Feb 1, 2021

View reviewed changes

Ali-D-Akbar force-pushed the aakbar/PROD-2132 branch from e893068 to 1fce864 Compare February 1, 2021 14:01

DawoudSheraz approved these changes Feb 1, 2021

View reviewed changes

add ability to expire entitlements

2fe77b5

Ali-D-Akbar force-pushed the aakbar/PROD-2132 branch from 1fce864 to 2fe77b5 Compare February 1, 2021 14:13

Ali-D-Akbar merged commit f77a4fc into master Feb 1, 2021

		@@ -221,24 +221,11 @@ export async function getCourseData(courseUUID) {
		}

		export async function patchEntitlement({

Add ability to expire Entitlements #83

Add ability to expire Entitlements #83

Conversation

Ali-D-Akbar commented Jan 21, 2021 • edited Loading

Add ability to expire entitlements in support tool

codecov bot commented Jan 21, 2021 • edited Loading

Codecov Report

saadyousafarbi left a comment

Choose a reason for hiding this comment

Ali-D-Akbar commented Jan 22, 2021 • edited Loading

awaisdar001 left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaisdar001 left a comment

Choose a reason for hiding this comment

DawoudSheraz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz left a 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

Ali-D-Akbar commented Jan 21, 2021 •

edited

Loading

codecov bot commented Jan 21, 2021 •

edited

Loading

Ali-D-Akbar commented Jan 22, 2021 •

edited

Loading