Add Client API #17583

Mjloturco · 2018-02-28T21:21:05Z

Adds a client api for the entitlement support tool front end to call to interact with entitlement support details endpoint

To be merged into this feature branch when approved

(part of) Learner-3925
Learner-4388

edx-status-bot · 2018-02-28T22:24:12Z

Your PR has finished running tests.

jaebradley · 2018-03-01T18:55:51Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+
+import endpoints from './endpoints';
+
+export function requestEntitlements(username_or_email) {


camelCase

for the param?

jaebradley · 2018-03-01T18:56:11Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+import endpoints from './endpoints';
+
+export function requestEntitlements(username_or_email) {
+	console.log('called requestEntitlements api function');


Is this console statement necessary?

ha, no . hadn't pushed that change

jaebradley · 2018-03-01T19:01:01Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  );
+}
+
+export function createEntitlement(course_uuid, user, mode, reason, comments) {


I've been finding that using objects as method parameters makes things, in some cases, easier to reason / read about when that method is called.

Here's an article that provides some examples.

// might be more readable createEntitlement({ courseUUID: '098sfgoj5', user: 'baejadley', mode: 'baemode', reason: 'tobebae', comments: 'jaebaebae' }); // than createEntitlement('098sfgoj5', 'baejadley', 'baemode', 'tobebae', 'jaebaebae' );

jaebradley · 2018-03-01T20:10:10Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/endpoints.js

@@ -0,0 +1,3 @@
+module.exports = {


Is there any reason not to use a named export?

export { entitlementList: 'whatever', inTheFutureSomeOtherEndpoint: 'moar whatever', };

it's going to cause an eslint error (maybe) if there's only one exported property, but I think it's fine to disable for now.

This article discusses the difference between CommonJS and ES6 modules.

Confirmed with @cpennington that we support ES6 and CommonJS import and exporting in edx-platform - I think we should use ES6 modules where we can.

edx-status-bot · 2018-03-01T20:10:31Z

Your PR has finished running tests.

jaebradley · 2018-03-01T20:12:58Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+import 'whatwg-fetch'; // fetch polyfill
+import Cookies from 'js-cookie';
+
+import endpoints from './endpoints';


There might be added benefit importing it this way so that you're forced to specify endpoints.entitlementList later on (and make it abundantly clear that it's some endpoint value) but this file is not so big that importing the value like

import { entitlementList } from './endpoints';

would be hard to decipher.

jaebradley · 2018-03-01T20:16:05Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+
+import endpoints from './endpoints';
+
+export function requestEntitlements({usernameOrEmail}) {


...maybe userIdentifier instead of usernameOrEmail?

#namingIsHard

I'm going to leave it as usernameOrEmail, since thats explicit, userIdentifier makes me think of a uuid.

jaebradley · 2018-03-01T20:17:30Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+}
+
+export function updateEntitlement({email, reason, entitlementUuid, comments}) {
+  //Only requires an 'email' parameter to construct the url, not actually sent in the body


Superfluous comment?

Relevant, my team has discussed modifying the api to not be built in the url pattern.

jaebradley · 2018-03-01T20:18:13Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+    `${endpoints.entitlementList}/${email}`, {
+      credentials: 'same-origin',
+      method: 'put',
+      headers:{


POST and PUT headers seem to be identical - can we DRY here?

jaebradley · 2018-03-01T20:20:23Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

@@ -0,0 +1,54 @@
+import 'whatwg-fetch'; // fetch polyfill


You probably copy/pasted this comment from somewhere, but it seems somewhat unnecessary?

jaebradley · 2018-03-01T20:42:17Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+      headers: headers,
+      body:JSON.stringify({
+      	entitlement_uuid: entitlementUuid,
+        reason: reason,


I hate myself for pointing this out but...indentation

jaebradley · 2018-03-01T20:43:10Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+      credentials: 'same-origin',
+      method: 'put',
+      headers: headers,
+      body:JSON.stringify({


Again, I hate myself for pointing this out but there should be a space before JSON.

jaebradley · 2018-03-01T20:48:54Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/endpoints.js

@@ -0,0 +1,4 @@
+export {
+  entitlementList: 'whatever',


probably don't want this to actually be whatever?

edx-status-bot · 2018-03-01T21:41:53Z

Your PR has finished running tests.

staubina

Overall looks good. Some minor code style comments and should be ready to

staubina · 2018-03-05T16:16:21Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+
+import { entitlementList } from './endpoints';
+
+const headers = {


Code Style Note: When defining a constant use all caps and underscores for the name (e.g. NAME_OF_CONST). It will clearly identify what in the code is constant and what is variable.

staubina · 2018-03-05T16:33:54Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/endpoints.js

@@ -0,0 +1,3 @@
+export {
+  entitlementList: '/support/entitlement_list',


This seems to be a constant at its root as well. You might want to make this all caps as well to match the code style.

I am researching this within out code style guides. This is the standard I have worked with in the past, but I am not sure where this fits in the edX style guide.

I have always found it to be more clear and helpful in the past.

I was modeling this off of studio front end endpoints. I'm not sure SCREAMING_SNAKE_CASE is out standard for constants in front end. @edx/fedx-team do we have a decision/opinion on that?

I am fine with what ever the style guide says :D. I have seen it different depending on the company.

I think studio-frontend might have a mistake there. The JS convention is to use the SCREAMING_SNAKE_CASE 🐍 for constants.

@Mjloturco and @staubina I think we default to the airbnb/javascript style guide. Currently, it looks like the rule is that constants should be defined as camelCase.

However, there is this PR that might add SCREAMING_SNAKE_CASE.

Got a couple of other opinions. You can disregard these recommendations. This is more a thing in Python and other languages.

Oh, guess I'm wrong. I thought our style guide was different. Redux uses SCREAMING_SNAKE_CASE for action name constants so I thought it was the same as that.

Cool, leaving it as is.

staubina · 2018-03-05T16:41:13Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+}
+
+export function updateEntitlement({email, reason, entitlementUuid, comments}) {
+  //Email param may be removable when EntitlementSupportListView


Can we add a ticket id to this that we are planning to do this work in? It looks like this should be tracked as a Task or story and a TODO.

We haven't decided yet if its something we want to do/adds any value

Should I make a ticket we can talk about in grooming?

Lets talk about it before grooming. If we believe it is work we should do it is better to have it in a ticket than lost in the code simply in a comment.

McKenzieW · 2018-03-05T16:42:03Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  'X-CSRFToken': Cookies.get('csrftoken'),
+}
+
+export function requestEntitlements({usernameOrEmail}) {


It would be clearer for the usernameOrEmail / user / email parameters in the three different functions to all have the one name, since they represent the same piece of information in each of the three requests.

McKenzieW · 2018-03-05T16:44:54Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  );
+}
+
+export function createEntitlement({courseUuid, user, mode, reason, comments}) {


Nit: for consistency it might be nice to have the ordering of parameters in the PUT and POST methods be fairly the same, i.e {user, reason, courseUuid, mode, comments} here to mirror below or {entitlementUuid, email, reason, comments} below to mirror what is here.

edx-status-bot · 2018-03-05T17:22:18Z

Your PR has finished running tests.

jaebradley · 2018-03-07T00:01:41Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+
+export function requestEntitlements({username}) {
+  return fetch(
+    `${entitlementApi}/?user=${usernameOrEmail}`, {


username?

jaebradley · 2018-03-07T00:07:05Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+
+export function requestEntitlements({username}) {
+  return fetch(
+    `${entitlementApi}/?user=${usernameOrEmail}`, {


Apparently fetch doesn't come with built-in query parameter support

It's probably overkill, but you could consider using qs or querystring

jaebradley · 2018-03-07T00:14:22Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  'X-CSRFToken': Cookies.get('csrftoken'),
+}
+
+export function requestEntitlements({username}) {


Taking a step back...if this is in the entitlements/data/api/client.js file, is the entitlements part of all of these method names redundant / extraneous?

import { requestEntitlements, createEntitlement } from './entitlements/data/api/client'; import { get, create } from './entitlements/data/api/client'; import { get as requestEntitlements, create as createEntitlement, } from './entitlements/data/api/client';

I get (and appreciate) the added verbosity but I wonder if it's relatively unnecessary?

We discussed in person, this comment is for history. I've decided to leave this as is for now, In the future additional endpoints may be added to this client, though currently it only interacts with the entitlement api

Mjloturco · 2018-03-07T16:11:42Z

jenkins run all

jaebradley · 2018-03-07T16:58:05Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  'X-CSRFToken': Cookies.get('csrftoken'),
+}
+
+export function requestEntitlements({username}) {


Nit: Yeah, gonna be that person again - these exports should be grouped at the end of the file.

Check out the eslint-plugin-import#groups-exports rule.

The airbnb/javascript style-guide doesn't explicitly mention this but in airbnb/javascript#1275 one of the maintainer of the style-guide who is also one of the maintainers of eslint-plugin-import said

As far as positioning, there will soon be a linter rule in eslint-plugin-import that will require that all export statements are grouped together - so I'd recommend grouping them together and putting them at the bottom of the file.

edx-status-bot · 2018-03-07T17:05:16Z

Your PR has finished running tests.

edx-status-bot · 2018-03-07T20:10:23Z

Your PR has finished running tests.

jaebradley · 2018-03-07T20:13:13Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  );
+}
+
+const updateEntitlement = ({entitlementUuid, unenrolled_run, reason, comments}) => {


jaebradley · 2018-03-07T20:15:39Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  );
+}
+
+const updateEntitlement = ({entitlementUuid, unenrolledRun, reason, comments}) => {


reason is unused?

jaebradley · 2018-03-07T20:15:56Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/client.js

+  );
+}
+
+const createEntitlement = ({username, courseUuid, mode, reason, comments}) => {


reason is also unused here?

they should both be action and should be included in the support_details object

edx-status-bot · 2018-03-07T21:19:15Z

Your PR has finished running tests.

jaebradley · 2018-03-07T21:53:52Z

package.json

@@ -53,6 +54,7 @@
    "webpack": "2.7.0",
    "webpack-bundle-tracker": "0.2.1",
    "webpack-merge": "4.1.1",
+    "whatwg-fetch": "^2.0.3",


It looks like the pattern is to pin these to an exact version (note the lack of ^).

sure, I just ran a command the linter told me to run. Should I just cut the ^?

check out the --save-exact npm install option

edx-status-bot · 2018-03-07T23:00:36Z

Your PR has finished running tests.

jaebradley

Should theoretically just be fixing styling at this point (maybe tests?) so 👍

jaebradley · 2018-03-08T01:30:22Z

lms/djangoapps/support/static/support/jsx/entitlements/data/api/endpoints.js

@@ -0,0 +1,3 @@
+export {
+  entitlementApi: '/api/entitlements/v1/entitlements',


If this is causing you linting errors because if only one thing gets exported it wants it to be the default export consider moving the constant to client.js unless additional endpoint values are expected to be added in the near future (which, if I'm remembering correctly, is not the case?).

Learner-3925 Learner-4388

edx-status-bot · 2018-03-08T18:37:25Z

Your PR has finished running tests.

edx-pipeline-bot · 2018-03-08T20:51:17Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, March 09, 2018.

edx-pipeline-bot · 2018-03-09T15:40:26Z

EdX Release Notice: This PR has been deployed to the production environment.

This was referenced Feb 28, 2018

Feature branch for EntitlementSupportPage #17574

Closed

Add action creators #17584

Closed

Mjloturco requested review from arizzitano, staubina, jaebradley and McKenzieW March 1, 2018 18:45

jaebradley reviewed Mar 1, 2018

View reviewed changes

Mjloturco changed the base branch from MLoTurco/learner-3925-feature to master March 5, 2018 16:17

Mjloturco force-pushed the MLoTurco/learner-3925-feature-client-api branch from 852e5ce to 4712bad Compare March 5, 2018 16:27

staubina suggested changes Mar 5, 2018

View reviewed changes

McKenzieW suggested changes Mar 5, 2018

View reviewed changes

jaebradley reviewed Mar 7, 2018

View reviewed changes

Mjloturco force-pushed the MLoTurco/learner-3925-feature-client-api branch from cdec30f to cc4e858 Compare March 7, 2018 15:31

Mjloturco force-pushed the MLoTurco/learner-3925-feature-client-api branch from 44ecaa4 to aef9fb2 Compare March 7, 2018 15:55

Mjloturco force-pushed the MLoTurco/learner-3925-feature-client-api branch from aef9fb2 to 946caec Compare March 7, 2018 16:13

McKenzieW approved these changes Mar 7, 2018

View reviewed changes

jaebradley reviewed Mar 7, 2018

View reviewed changes

staubina approved these changes Mar 7, 2018

View reviewed changes

Mjloturco force-pushed the MLoTurco/learner-3925-feature-client-api branch from 946caec to 35a305c Compare March 7, 2018 19:19

jaebradley reviewed Mar 7, 2018

View reviewed changes

jaebradley approved these changes Mar 8, 2018

View reviewed changes

jaebradley reviewed Mar 8, 2018

View reviewed changes

Add client api for entitlement support page

a117f6f

Learner-3925 Learner-4388

Mjloturco force-pushed the MLoTurco/learner-3925-feature-client-api branch from a463286 to a117f6f Compare March 8, 2018 17:46

Mjloturco merged commit 0d07a89 into master Mar 8, 2018

jdmulloy deleted the MLoTurco/learner-3925-feature-client-api branch April 5, 2018 18:28


		import endpoints from './endpoints';

		export function requestEntitlements(username_or_email) {


		import endpoints from './endpoints';

		export function requestEntitlements({usernameOrEmail}) {


		import { entitlementList } from './endpoints';

		const headers = {

		@@ -0,0 +1,3 @@
		export {
		entitlementList: '/support/entitlement_list',

		@@ -0,0 +1,3 @@
		export {
		entitlementApi: '/api/entitlements/v1/entitlements',

Add Client API #17583

Add Client API #17583

Conversation

Mjloturco commented Feb 28, 2018 • edited

edx-status-bot commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mjloturco Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaebradley Mar 1, 2018 • edited

Choose a reason for hiding this comment

jaebradley Mar 1, 2018 • edited

Choose a reason for hiding this comment

jaebradley Mar 1, 2018 • edited

Choose a reason for hiding this comment

edx-status-bot commented Mar 1, 2018

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

edx-status-bot commented Mar 1, 2018

staubina left a comment

Choose a reason for hiding this comment

staubina Mar 5, 2018 • edited

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

jaebradley Mar 5, 2018 • edited

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

edx-status-bot commented Mar 5, 2018

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

Mjloturco Mar 7, 2018 • edited

Choose a reason for hiding this comment

Mjloturco commented Mar 7, 2018

Choose a reason for hiding this comment

edx-status-bot commented Mar 7, 2018

edx-status-bot commented Mar 7, 2018

jaebradley Mar 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaebradley Mar 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-status-bot commented Mar 7, 2018

jaebradley Mar 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-status-bot commented Mar 7, 2018

jaebradley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-status-bot commented Mar 8, 2018

Mjloturco commented Feb 28, 2018 •

edited

Mjloturco Mar 1, 2018 •

edited

jaebradley Mar 1, 2018 •

edited

jaebradley Mar 1, 2018 •

edited

jaebradley Mar 1, 2018 •

edited

staubina Mar 5, 2018 •

edited

jaebradley Mar 5, 2018 •

edited

Mjloturco Mar 7, 2018 •

edited

jaebradley Mar 7, 2018 •

edited

jaebradley Mar 7, 2018 •

edited

jaebradley Mar 7, 2018 •

edited