Added staff-to-learner email with link in learner chip #2693

gsidebo · 2017-02-27T05:39:17Z

What are the relevant tickets?

What's this PR do?

Implements the API endpoint to send a staff-to-learner email, and adds the link on the learner chip to launch the email dialog

How should this be manually tested?

From the /learners page, hover over a learner's name, click the email link, send an email, and repeat for a couple of other learners. Make sure MAILGUN_RECIPIENT_OVERRIDE is set to your email address

Where should the reviewer start?

LearnerResult.js and LearnerChip.js

codecov-io · 2017-02-27T05:39:18Z

Codecov Report

Merging #2693 into master will decrease coverage by -0.05%.
The diff coverage is 91.73%.

@@            Coverage Diff            @@
##           master   #2693      +/-   ##
=========================================
- Coverage   93.54%   93.5%   -0.05%     
=========================================
  Files         359     359              
  Lines       14742   14789      +47     
  Branches      650     651       +1     
=========================================
+ Hits        13790   13828      +38     
- Misses        899     905       +6     
- Partials       53      56       +3

Impacted Files	Coverage Δ
static/js/test_constants.js	`100% <ø> (ø)`	✅
static/js/components/email/hoc.js	`95.55% <ø> (-0.1%)`	❌
mail/urls.py	`100% <ø> (ø)`	✅
static/js/reducers/email_test.js	`100% <100%> (ø)`	✅
static/js/lib/api_test.js	`99.67% <100%> (ø)`	✅
static/js/components/email/constants.js	`100% <100%> (ø)`	✅
static/js/components/LearnerChip_test.js	`100% <100%> (ø)`	✅
static/js/containers/LearnerSearchPage.js	`83.33% <100%> (ø)`	✅
mail/views.py	`100% <100%> (ø)`	✅
static/js/components/LearnerSearch.js	`90.56% <100%> (+0.56%)`	✅
... and 15 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 8addbb1...e13528c. Read the comment docs.

singingwolfboy

This is not intended to be a full review of this pull request: I am not assigning myself to it. I just wanted to point out an accessibility problem that I found.

singingwolfboy · 2017-02-27T16:22:35Z

static/js/components/LearnerChip.js

+        <Icon name="person"/>
+        <span>View Profile</span>
+      </a>
+      <a onClick={openLearnerEmailComposer} className="mm-minor-action">


This should be a <button> element, for accessibility reasons. See #1222.

singingwolfboy · 2017-02-27T16:26:28Z

static/scss/user-chip.scss

@@ -32,6 +32,7 @@

    a:hover {
      text-decoration: none !important;
+      cursor: pointer;


If this is only because of the <a onClick> element referenced above, then after changing it to a <button> element, this line should be removed. (Or is there some other reason why this was added?)

gsidebo · 2017-02-28T16:06:01Z

@roberthouse54 here are some screenshots. let me know what you think:

singingwolfboy · 2017-03-01T14:16:47Z

static/js/components/search/LearnerResult_test.js

+      // Execute the openLearnerEmailComposer function passed down to the LearnerChip
+      result.find("LearnerChip").props().openLearnerEmailComposer();
+      assert.isTrue(openLearnerEmailComposerStub.called);
+      assert.isTrue(openLearnerEmailComposerStub.calledWith(USER_PROFILE_RESPONSE));


I recommend using sinon.assert.called() and sinon.assert.calledWith() instead of assert.isTrue() for these cases. You get a much clearer failure message if the assertion fails. http://sinonjs.org/releases/v1.17.7/assertions/

singingwolfboy · 2017-03-01T14:19:32Z

static/scss/shared-styles.scss

+  }
+
+  &.medium {
+    height: auto !important;


Why do you need !important here?

this was copied from the original style. not sure i want to find out why it's there as a part of this PR

Gotcha. @roberthouse54, can you take a look at this? I've created #2732 to address this and other issues.

singingwolfboy · 2017-03-01T14:21:29Z

mail/views.py

+        sender_user = request.user
+        recipient_user = self.get_object().user
+        serializer = self.get_serializer(data=request.data)
+        serializer.is_valid(raise_exception=True)


Please add a test to cover an invalid serialization error.

singingwolfboy · 2017-03-01T14:23:27Z

micromasters/urls_test.py

@@ -20,7 +20,7 @@ def test_urls(self):
        assert reverse('checkout') == '/api/v0/checkout/'
        assert reverse('user_program_enrollments') == '/api/v0/enrolledprograms/'
        assert reverse('user_course_enrollments') == '/api/v0/course_enrollments/'
-        assert reverse('search_result_mail_api') == '/api/v0/mail/'
+        assert reverse('search_result_mail_api') == '/api/v0/mail/search/'


It probably also makes sense to add a test for learner_mail_api

singingwolfboy · 2017-03-01T14:24:38Z

static/js/actions/email.js

@@ -31,6 +31,23 @@ export const sendEmailSuccess = createAction(SEND_EMAIL_SUCCESS);
 export const SEND_EMAIL_FAILURE = 'SEND_EMAIL_FAILURE';
 export const sendEmailFailure = createAction(SEND_EMAIL_FAILURE);

+export function sendEmail(
+  emailType: string,


Is emailType an arbitrary string, or does it have to be one of a short list of strings? If the latter, please specify that list in the Flow type.

it can theoretically be any string. static/js/components/email/constants.js has a short list of strings that we can use for known email types, but this argument to sendEmail just depends on the keys passed into the withEmailDialog higher-order component (see: static/js/containers/DashboardPage.js:732). those keys can be anything

singingwolfboy · 2017-03-01T14:34:34Z

static/js/components/search/LearnerResult_test.js

+      result.find(".learner-name").props().onMouseEnter();
+    }).then(() => {
+      // Execute the openLearnerEmailComposer function passed down to the LearnerChip
+      result.find("LearnerChip").props().openLearnerEmailComposer();


Can you stick in an assertion, to be sure that we find the LearnerChip? Something like this:

const chip = result.find("LearnerChip"); assert.lengthOf(chip, 1); chip.props().openLearnerEmailCompose();

The way this is currently written, if the LearnerChip is not rendered, you'll get a misleading error message about calling .props() on nothing. This will make that failure case more clear.

singingwolfboy · 2017-03-01T14:39:01Z

static/js/flow/emailTypes.js

@@ -31,5 +29,5 @@ export type EmailConfig = {
  title: string,
  renderSubheading: (subheading: string) => React$Element<*>,
  emailOpenParams: (args: any) => Object,
-  emailSendAction: (emailState: EmailState) => Dispatcher<EmailSendResponse>
+  emailSendParams: (emailState: EmailState) => Array<any>


We should avoid using the any type as much as possible. What type or types should this be?

it can literally be anything. for one implementation, that array contains strings and a complex ES query object. for another implementation, it contains strings and an integer ID

singingwolfboy · 2017-03-01T14:40:45Z

static/js/lib/api_test.js

+      it('returns expected values when a POST to send a learner email succeeds', () => {
+        fetchJSONStub.returns(Promise.resolve(MAIL_RESPONSE));
+        return sendLearnerMail('subject', 'body', learnerStudentId).then(mailResp => {
+          assert.ok(fetchJSONStub.calledWith(`/api/v0/mail/learner/${learnerStudentId}/`, {


It would be better to use sinon.assert.calledWith().

singingwolfboy · 2017-03-01T14:41:58Z

static/js/util/util.js

@@ -517,6 +518,20 @@ export function getDisplayName(WrappedComponent: ReactClass<*>) {
  return WrappedComponent.displayName || WrappedComponent.name || 'Component';
 }

+export const wrapWithProps = (addedProps: Object, WrappedComponent: ReactClass<*>) => {


Nitpick: can you add Flow type information that this function returns a ReactClass?

singingwolfboy · 2017-03-01T14:43:15Z

mail/views_test.py

+        self.client.force_login(self.staff_user)
+        mock_mailgun_client.send_individual_email.return_value = Mock(
+            spec=Response,
+            status_code=status.HTTP_200_OK,


Can you add another test simulating a failure from Mailgun?

we don't currently have tests that simulate a mailgun failure for any of the other email views. it will involve patching requests.post or something like that. i made a quick attempt at it and failed. it's non-trivial and i would very strongly prefer creating a separate issue for that work. if you have any objections let me know

You don't need to patch the requests module, just do the same mocking that you're doing here, but have it return 400 Bad Request status code. It's worth verifying that, if Mailgun returns a non-successful HTTP response, then our code should also return a non-successful HTTP response.

singingwolfboy · 2017-03-01T15:05:52Z

There's a usability problem with this feature, although fixing it might be outside the scope of this pull request. If you open the "Send a Message" dialog, type in a message, close the dialog without sending the message, and reopen it, the message you've typed in is gone. These sorts of dialogs are easy to close accidentally, just by clicking outside of the dialog. I could also imagine an instructor writing a message to a student, closing the dialog to check the student's grade in the program, and then reopening the dialog expecting the message to be saved.

Can you save the contents of the message when the dialog is closed, and repopulate the dialog with that message when it's reopened? The saved contents would only get cleared after the email is successfully sent, which would help deal with Mailgun failures. If Mailgun is experiencing a temporary failure, the instructor doesn't have to type out the whole message again -- just reopen the dialog and click send again!

singingwolfboy · 2017-03-01T14:57:20Z

static/js/components/LearnerChip.js

@@ -18,12 +20,16 @@ const LearnerChip = ({ profile }: {profile: Profile}): React$Element<*> => (
        { mstr(getEmployer(profile)) }
      </span>
      <a href={`/learner/${profile.username}`} className="mm-minor-action">
-        <Icon name="person" />
-        <span>View profile</span>
+        <Icon name="person"/>


Since this link is already described by the text View Profile, please add aria-hidden=true on this <Icon>.

singingwolfboy · 2017-03-01T14:57:26Z

static/js/components/LearnerChip.js

-  </Card>
-);
+      <button onClick={openLearnerEmailComposer} className="mm-minor-action">
+        <Icon name="email"/>


Since this button is already described by the text Send a Message, please add aria-hidden=true on this <Icon>.

singingwolfboy · 2017-03-01T15:10:09Z

Also, I don't know if we care about viewing this dialog on mobile, but the layout is wrapping when it doesn't need to.

noisecapella · 2017-03-01T15:31:27Z

mail/views_test.py

@@ -58,7 +58,7 @@ def setUpTestData(cls):
        }

    def setUp(self):
-        super(MailViewsTests, self).setUp()
+        super(SearchResultMailViewsTests, self).setUp()


Not reviewing, but you can make this just super().setUp() in Python 3

gsidebo · 2017-03-01T15:47:39Z

@singingwolfboy regarding the clearing of the dialog when you cancel out of it, this has been a 'feature' as long as we've had the email dialogs. i agree that the input should be saved. i'll open a separate issue for it

gsidebo · 2017-03-01T19:18:39Z

@singingwolfboy should be all set

noisecapella · 2017-03-03T18:29:24Z

I see the email link on the learner chip, should there also be one on the learner profile page?

noisecapella · 2017-03-03T18:38:21Z

mail/views.py

+        authentication.SessionAuthentication,
+        authentication.TokenAuthentication,
+    )
+    permission_classes = (permissions.IsAuthenticated, UserCanMessageLearnersPermission, )


We need to also verify that the staff is staff of a program that the user is enrolled in. We don't have an extra permission in the search view because we add a filter on the programs the staff is enrolled in via Elasticsearch, but here we should check this in a permission class

noisecapella · 2017-03-03T18:49:33Z

mail/views_test.py

@@ -252,6 +252,72 @@ def test_course_team_email_unpaid(self):
        assert resp.status_code == status.HTTP_403_FORBIDDEN


+class LearnerMailViewTests(APITestCase, MockedESTestCase):


Can you add a test where a user who is not staff attempts to use this API to send an email?

noisecapella · 2017-03-03T18:53:44Z

Functionality looks great

noisecapella · 2017-03-03T19:08:26Z

static/js/components/LearnerSearch.js

+  constructor(props: Object) {
+    super(props);
+    this.WrappedLearnerResult = wrapWithProps(
+      {openLearnerEmailComposer: this.props.openLearnerEmailComposer},


I don't think it's a good idea to set this in the constructor. What if openLearnerEmailComposer changes later?

I did this because there was a visually-noticeable performance penalty if the component wrapping is performed in, say, the render method. It's also important to note that this method can't change (at least not through any means I'm aware of). It's handed down from the email HOC, which is only executed once on the container component, and the function is not part of the state, so it can't change via an action/reducer.

I agree that this is kind of a code smell and it isn't by-the-book React, but I don't quite know how else to handle this without repeatedly re-wrapping this component unnecessarily.

It would be nice to investigate what the performance penalty was, but feel free to leave as is for this PR

noisecapella · 2017-03-03T19:36:34Z

static/js/components/email/lib.js

+  ])
+};
+
+export const LEARNER_EMAIL_CONFIG: EmailConfig = {


Can you add tests which cover these functions, either via unit tests and mocks or with an integration test?

gsidebo · 2017-03-03T22:27:19Z

@noisecapella the profile page link will be implemented as part of #2196. Sorry, I had it in mind that I would just implement the learner chip link in this issue and the profile page link in that other issue, but forgot to update the descriptions. They are updated now

gsidebo · 2017-03-07T17:04:32Z

@noisecapella that should do it

noisecapella

Looks great 👍

@noisecapella

@noisecapella has taken over

gsidebo added the Needs Review label Feb 27, 2017

odlbot temporarily deployed to micromasters-ci-pr-2693 February 27, 2017 05:39 Inactive

singingwolfboy reviewed Feb 27, 2017

View reviewed changes

giocalitri assigned singingwolfboy Feb 27, 2017

odlbot temporarily deployed to micromasters-ci-pr-2693 February 27, 2017 18:18 Inactive

gsidebo force-pushed the 2641_staff_to_learner_email branch from 3d1cf5e to a9bac77 Compare February 27, 2017 18:18

odlbot temporarily deployed to micromasters-ci-pr-2693 February 27, 2017 18:18 Inactive

gsidebo added Work in Progress and removed Needs Review labels Feb 27, 2017

gsidebo force-pushed the 2641_staff_to_learner_email branch from a9bac77 to e68b921 Compare February 27, 2017 20:51

odlbot temporarily deployed to micromasters-ci-pr-2693 February 27, 2017 20:51 Inactive

odlbot temporarily deployed to micromasters-ci-pr-2693 February 27, 2017 21:45 Inactive

gsidebo added Needs Review and removed Work in Progress labels Feb 27, 2017

odlbot temporarily deployed to micromasters-ci-pr-2693 February 28, 2017 15:58 Inactive

gsidebo force-pushed the 2641_staff_to_learner_email branch from 4ed9fdf to ce0059c Compare February 28, 2017 21:31

odlbot had a problem deploying to micromasters-ci-pr-2693 March 1, 2017 00:14 Failure

odlbot temporarily deployed to micromasters-ci-pr-2693 March 1, 2017 02:05 Inactive

singingwolfboy suggested changes Mar 1, 2017

View reviewed changes

singingwolfboy previously requested changes Mar 1, 2017

View reviewed changes

noisecapella reviewed Mar 1, 2017

View reviewed changes

pdpinch mentioned this pull request Mar 1, 2017

Remove "!important" CSS rules as much as possible #2732

Open

gsidebo force-pushed the 2641_staff_to_learner_email branch from ce0059c to a42cd00 Compare March 1, 2017 18:55

odlbot temporarily deployed to micromasters-ci-pr-2693 March 1, 2017 18:56 Inactive

gsidebo force-pushed the 2641_staff_to_learner_email branch from 0b51ee8 to 3ba0cb0 Compare March 3, 2017 18:15

odlbot temporarily deployed to micromasters-ci-pr-2693 March 3, 2017 18:16 Inactive

noisecapella reviewed Mar 3, 2017

View reviewed changes

odlbot temporarily deployed to micromasters-ci-pr-2693 March 3, 2017 18:42 Inactive

noisecapella reviewed Mar 3, 2017

View reviewed changes

noisecapella added Waiting on Author and removed Needs Review labels Mar 3, 2017

noisecapella suggested changes Mar 3, 2017

View reviewed changes

noisecapella mentioned this pull request Mar 3, 2017

Refactor MailgunClient for better error handling #2775

Merged

gsidebo force-pushed the 2641_staff_to_learner_email branch from 8a2bf99 to 74b9b2a Compare March 6, 2017 16:25

odlbot temporarily deployed to micromasters-ci-pr-2693 March 6, 2017 16:25 Inactive

odlbot temporarily deployed to micromasters-ci-pr-2693 March 7, 2017 17:01 Inactive

gsidebo added Needs Review and removed Waiting on Author labels Mar 7, 2017

gsidebo force-pushed the 2641_staff_to_learner_email branch from 42840fb to 42f1765 Compare March 7, 2017 18:50

odlbot temporarily deployed to micromasters-ci-pr-2693 March 7, 2017 18:50 Inactive

noisecapella approved these changes Mar 7, 2017

View reviewed changes

noisecapella added Waiting on Author and removed Needs Review labels Mar 7, 2017

Added staff-to-learner email with link in learner chip

e13528c

gsidebo force-pushed the 2641_staff_to_learner_email branch from 42f1765 to e13528c Compare March 7, 2017 20:13

odlbot deployed to micromasters-ci-pr-2693 March 7, 2017 20:13 View deployment

gsidebo merged commit b281c13 into master Mar 7, 2017

gsidebo deleted the 2641_staff_to_learner_email branch March 7, 2017 21:36

		@@ -252,6 +252,72 @@ def test_course_team_email_unpaid(self):
		assert resp.status_code == status.HTTP_403_FORBIDDEN


		class LearnerMailViewTests(APITestCase, MockedESTestCase):

Added staff-to-learner email with link in learner chip #2693

Added staff-to-learner email with link in learner chip #2693

Conversation

gsidebo commented Feb 27, 2017

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Where should the reviewer start?

codecov-io commented Feb 27, 2017 • edited Loading

Codecov Report

singingwolfboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsidebo commented Feb 28, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsidebo Mar 1, 2017 • 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

singingwolfboy commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singingwolfboy commented Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

gsidebo commented Mar 1, 2017

gsidebo commented Mar 1, 2017

noisecapella commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisecapella commented Mar 3, 2017

Choose a reason for hiding this comment

gsidebo Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsidebo commented Mar 3, 2017

gsidebo commented Mar 7, 2017

noisecapella left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 27, 2017 •

edited

Loading

gsidebo commented Feb 28, 2017 •

edited

Loading

gsidebo Mar 1, 2017 •

edited

Loading

singingwolfboy commented Mar 1, 2017 •

edited

Loading

gsidebo Mar 3, 2017 •

edited

Loading