Repository: zooniverse/Panoptes-Front-End (js)

Issue:
Password reset form swallows API errors
https://github.com/zooniverse/Panoptes-Front-End/blob/d6c8a183e3a8e6dfe653f426c77e9107a240e446/app/pages/reset-password/new-password-form.jsx#L48-L53

The original error message should be shown here, instead of a generic "Something went wrong" error that doesn't tell volunteers what they did wrong.

Pull request:
add email password reset form errors
Staging branch URL:  https://email-reset-errors.pfe-preview.zooniverse.org/reset-password?reset_password_token=H6GtX

Fixes #4390 and lots of email reports of problems with email password resets, specifically no API error reports, no length checking or password match checks. 

This PR adds:
1. the API failure messages to the form
0. HTML5 input password length validation before form submission
0. Checks the passwords match before attempting to contact the API.

# Required Manual Testing

- [x] Does the non-logged in home page render correctly?
- [x] Does the logged in home page render correctly?
- [x] Does the projects page render correctly?
- [x] Can you load project home pages?
- [x] Can you load the classification page?
- [ ] Can you submit a classification?
- [x] Does talk load correctly?
- [ ] Can you post a talk comment?

# Review Checklist

- [x] Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
- [ ] Does it work on mobile?
- [x] Can you `rm -rf node_modules/ && npm install` and app works as expected?
- [ ] If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
- [x] Are the tests passing locally and on Travis?

# Optional

- [ ] Have you replaced any `ChangeListener` or `PromiseRenderer` components with code that updates component state?
- [ ] If changes are made to the classifier, does the dev classifier still work?
- [ ] Have you [resized and compressed](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/image-optimization) any image you've added?
- [ ] Have you added in [flow type annotations](https://flowtype.org/docs/type-annotations.html)?
- [ ] Have you followed the [Springer guidelines for commit messages](https://github.com/springernature/frontend-playbook/blob/master/git/git.md#commit-messages)?


Patch:
diff --git a/app/lib/classification-queue.js b/app/lib/classification-queue.js
new file mode 100644
index 00000000..dffcedab
--- /dev/null
+++ b/app/lib/classification-queue.js
@@ -0,0 +1,98 @@
+/* eslint no-underscore-dangle: ["error", {"allowAfterThis": true}] */
+import apiClient from 'panoptes-client/lib/api-client';
+
+const FAILED_CLASSIFICATION_QUEUE_NAME = 'failed-classifications';
+const MAX_RECENTS = 10;
+
+class ClassificationQueue {
+  constructor(storage, api, onClassificationSaved) {
+    this.storage = storage || window.localStorage;
+    this.apiClient = api || apiClient;
+    this.recents = [];
+    this.onClassificationSaved = onClassificationSaved;
+  }
+
+  add(classification) {
+    const queue = this._loadQueue();
+    queue.push(classification);
+
+    try {
+      this._saveQueue(queue);
+      console.info('Queued classifications:', queue.length);
+    } catch (error) {
+      console.error('Failed to queue classification:', error);
+    }
+
+    this.flushToBackend();
+  }
+
+  length() {
+    return this._loadQueue().length;
+  }
+
+  addRecent(classification) {
+    if (this.recents.length > MAX_RECENTS) {
+      const droppedClassification = this.recents.pop();
+      droppedClassification.destroy();
+    }
+
+    this.recents.unshift(classification);
+  }
+
+  flushToBackend() {
+    const queue = this._loadQueue();
+
+    if (queue.length > 0) {
+      console.log('Saving queued classifications:', queue.length);
+      queue.forEach((classificationData) => {
+        this.apiClient.type('classifications').create(classificationData).save().then((actualClassification) => {
+          console.log('Saved classification', actualClassification.id);
+          this.onClassificationSaved(actualClassification);
+          this.addRecent(actualClassification);
+
+          const indexInQueue = queue.indexOf(classificationData);
+          queue.splice(indexInQueue, 1);
+
+          try {
+            this._saveQueue(queue);
+            console.info('Saved a queued classification, remaining:', queue.length);
+          } catch (error) {
+            console.error('Failed to update classification queue:', error);
+          }
+        })
+        .catch((error) => {
+          console.error('Failed to save a queued classification:', error);
+
+          if (error.status === 422) {
+            console.error('Dropping malformed classification permanently', classificationData);
+            const indexInQueue = queue.indexOf(classificationData);
+            queue.splice(indexInQueue, 1);
+
+            try {
+              this._saveQueue(queue);
+            } catch (saveQueueError) {
+              console.error('Failed to update classification queue:', saveQueueError);
+            }
+          }
+        });
+      });
+    }
+  }
+
+  _loadQueue() {
+    let queue = JSON.parse(this.storage.getItem(FAILED_CLASSIFICATION_QUEUE_NAME));
+
+    if (queue === undefined || queue === null) {
+      queue = [];
+    }
+
+    return queue;
+  }
+
+  _saveQueue(queue) {
+    this.storage.setItem(FAILED_CLASSIFICATION_QUEUE_NAME, JSON.stringify(queue));
+  }
+
+}
+
+export default ClassificationQueue;
diff --git a/app/lib/classification-queue.spec.js b/app/lib/classification-queue.spec.js
new file mode 100644
index 00000000..36bac92e
--- /dev/null
+++ b/app/lib/classification-queue.spec.js
@@ -0,0 +1,29 @@
+import assert from 'assert';
+import ClassificationQueue from './classification-queue';
+import FakeLocalStorage from '../../test/fake-local-storage';
+import { FakeApiClient } from '../../test/fake-api-client';
+
+describe('ClassificationQueue', function() {
+  it('sends classifications to the backend', function() {
+    let apiClient = new FakeApiClient();
+    let storage = new FakeLocalStorage();
+
+    let classificationData = {annotations: [], metadata: {}};
+    let classificationQueue = new ClassificationQueue(storage, apiClient);
+    classificationQueue.add(classificationData);
+
+    assert.equal(apiClient.saves.length, 1);
+  });
+
+  it('keeps classifications in localStorage if backend fails', function() {
+    let apiClient = new FakeApiClient({canSave: () => { return false; }});
+    let storage = new FakeLocalStorage();
+
+    let classificationData = {annotations: [], metadata: {}};
+    let classificationQueue = new ClassificationQueue(storage, apiClient);
+    classificationQueue.add(classificationData);
+
+    assert.equal(apiClient.saves.length, 0);
+    assert.equal(classificationQueue.length(), 1);
+  });
+});
diff --git a/app/locales/en.js b/app/locales/en.js
index 45eaceba..b0a8dfbb 100644
--- a/app/locales/en.js
+++ b/app/locales/en.js
@@ -58,7 +58,7 @@ export default {
         subjects: 'Subjects'
       },
       talk: {
-        zero: 'Noone is talking about <strong>%(title)s</strong> right now.',
+        zero: 'No one is talking about <strong>%(title)s</strong> right now.',
         one: '<strong>1</strong> person is talking about <strong>%(title)s</strong> right now.',
         other: '<strong>%(count)s</strong> people are talking about <strong>%(title)s</strong> right now.'
       },
@@ -168,13 +168,13 @@ export default {
   },
   resetPassword: {
     heading: 'Reset Password',
-    newPasswordFormDialog: 'Go ahead and enter a new password, then you can get back to doing some research.',
+    newPasswordFormDialog: 'Enter the same password twice so you can get back to doing some research. Passwords need to be at least 8 characters long.',
     newPasswordFormLabel: 'New password:',
-    newPasswordConfirmationLabel: 'Again, to confirm:',
+    newPasswordConfirmationLabel: 'Repeat your password to confirm:',
     enterEmailLabel: 'Please enter your email address here and we’ll send you a link you can follow to reset it.',
     emailSuccess: 'We’ve just sent you an email with a link to reset your password.',
     emailError: 'There was an error resetting your password.',
-    resetError: 'Something went wrong, please try and reset your password via email again.',
+    passwordsDoNotMatch: 'The passwords do not match, please try again.',
     loggedInDialog: 'You are currently logged in. Please log out if you would like to reset your password.'
   },
   workflowToggle: {
diff --git a/app/locales/es.js b/app/locales/es.js
index ae1c044f..342f059e 100644
--- a/app/locales/es.js
+++ b/app/locales/es.js
@@ -156,13 +156,14 @@ export default {
   },
   resetPassword: {
     heading: 'Reset Password',
-    newPasswordFormDialog: 'Go ahead and enter a new password, then you can get back to doing some research.',
+    newPasswordFormDialog: 'Enter the same password twice so you can get back to doing some research. Passwords need to be at least 8 characters long.',
     newPasswordFormLabel: 'New password:',
-    newPasswordConfirmationLabel: 'Again, to confirm:',
+    newPasswordConfirmationLabel: 'Repeat your password to confirm:',
     enterEmailLabel: 'Please enter your email address here and we’ll send you a link you can follow to reset it.',
     emailSuccess: 'We’ve just sent you an email with a link to reset your password.',
     emailError: 'There was an error reseting your password.',
     resetError: 'Something went wrong, please try and reset your password via email again.',
+    passwordsDoNotMatch: 'The passwords do not match, please try again.',
     loggedInDialog: 'You are currently logged in. Please log out if you would like to reset your password.'
   },
   workflowToggle: {
diff --git a/app/locales/nl.js b/app/locales/nl.js
index e8228f55..2a9e98b9 100644
--- a/app/locales/nl.js
+++ b/app/locales/nl.js
@@ -189,13 +189,13 @@ export default {
   },
   resetPassword: {
     heading: 'Wachtwoord vergeten',
-    newPasswordFormDialog: 'Voer een nieuw wachtwoord in, dan kun je verder met onderzoek doen.',
+    newPasswordFormDialog: 'Voer hetzelfde wachtwoord tweemaal in, zodat je verder kunt met onderzoek doen. Een wachtwoord moet tenminste 8 tekens lang zijn.',
     newPasswordFormLabel: 'Nieuw wachtwoord:',
     newPasswordConfirmationLabel: 'Herhaal, ter controle:',
     enterEmailLabel: 'Voer je e-mailadres in en we sturen je een link waarmee je kunt herstellen.',
     emailSuccess: 'We hebben je zojuist een e-mail met een herstellink gezonden.',
     emailError: 'Er ging iets mis bij het herstellen van je wachtwoord.',
-    resetError: 'Er ging iets mis, probeer het nogmaals.',
+    passwordsDoNotMatch: 'De wachtwoorden komen niet overeen, probeer het opnieuw.',
     loggedInDialog: 'Je bent op dit moment ingelogd. Log uit om je wachtwoord te herstellen.'
   },
   workflowToggle: {
diff --git a/app/pages/project/classify.cjsx b/app/pages/project/classify.cjsx
index eefb929e..79a71b56 100644
--- a/app/pages/project/classify.cjsx
+++ b/app/pages/project/classify.cjsx
@@ -9,6 +9,7 @@ counterpart = require 'counterpart'
 `import FinishedBanner from './finished-banner';`
 Classifier = require('../../classifier').default
 seenThisSession = require '../../lib/seen-this-session'
+ClassificationQueue = require('../../lib/classification-queue').default
 `import WorkflowAssignmentDialog from '../../components/workflow-assignment-dialog';`
 { Split } = require('seven-ten')
 
@@ -16,8 +17,6 @@ counterpart.registerTranslations 'en',
   classifyPage:
     title: 'Classify'
 
-FAILED_CLASSIFICATION_QUEUE_NAME = 'failed-classifications'
-
 # Map each project ID to a promise of its last randomly-selected workflow ID.
 # This is to maintain the same random workflow for each project when none is specified by the user.
 currentWorkflowForProject = {}
@@ -39,6 +38,11 @@ emptySubjectQueue = ->
       subject.destroy()
     queue.splice 0
 
+onClassificationSaved = (actualClassification) ->
+  Split.classificationCreated(actualClassification); # Metric log needs classification id
+
+classificationQueue = new ClassificationQueue(window.localStorage, apiClient, onClassificationSaved)
+
 auth.listen 'change', emptySubjectQueue
 apiClient.type('subject_sets').listen 'add-or-remove', emptySubjectQueue
 
@@ -260,48 +264,10 @@ module.exports = createReactClass
 
     {workflow, subjects} = classification.links
     seenThisSession.add workflow, subjects
-    @queueClassification classification unless @state.demoMode
-    @saveAllQueuedClassifications()
+    unless @state.demoMode
+      classificationQueue.add(classification)
     Promise.resolve classification
 
-  queueClassification: (classification) ->
-    queue = JSON.parse localStorage.getItem FAILED_CLASSIFICATION_QUEUE_NAME
-    queue ?= []
-    queue.push classification
-    try
-      localStorage.setItem FAILED_CLASSIFICATION_QUEUE_NAME, JSON.stringify queue
-      console?.info 'Queued classifications:', queue.length
-    catch error
-      console?.error 'Failed to queue classification:', error
-
-  saveAllQueuedClassifications: ->
-    queue = JSON.parse localStorage.getItem FAILED_CLASSIFICATION_QUEUE_NAME
-    if queue? and queue.length isnt 0
-      console?.log 'Saving queued classifications:', queue.length
-      for classificationData in queue then do (classificationData) =>
-        apiClient.type('classifications').create(classificationData).save()
-          .then (actualClassification) =>
-            console?.log 'Saved classification', actualClassification.id
-            Split.classificationCreated(actualClassification) # Metric log needs classification id
-            actualClassification.destroy()
-            indexInQueue = queue.indexOf classificationData
-            queue.splice indexInQueue, 1
-            try
-              localStorage.setItem FAILED_CLASSIFICATION_QUEUE_NAME, JSON.stringify queue
-              console?.info 'Saved a queued classification, remaining:', queue.length
-            catch error
-              console?.error 'Failed to update classification queue:', error
-          .catch (error) =>
-            console?.error 'Failed to save a queued classification:', error
-            switch error.status
-              when 422
-                indexInQueue = queue.indexOf classificationData
-                queue.splice indexInQueue, 1
-                try
-                  localStorage.setItem FAILED_CLASSIFICATION_QUEUE_NAME, JSON.stringify queue
-                catch error
-                  console?.error 'Failed to update classification queue:', error
-
   loadAnotherSubject: ->
     # Forget the old classification so a new one will load.
     currentClassifications.forWorkflow[@props.workflow.id] = null
@@ -349,3 +315,4 @@ module.exports = createReactClass
 window.currentWorkflowForProject = currentWorkflowForProject
 window.currentClassifications = currentClassifications
 window.upcomingSubjects = upcomingSubjects
+window.classificationQueue = classificationQueue
diff --git a/app/pages/reset-password/new-password-form.jsx b/app/pages/reset-password/new-password-form.jsx
index eeed8ce2..58209598 100644
--- a/app/pages/reset-password/new-password-form.jsx
+++ b/app/pages/reset-password/new-password-form.jsx
@@ -2,7 +2,7 @@ import PropTypes from 'prop-types';
 import React from 'react';
 import Translate from 'react-translate-component';
 
-const NewPasswordForm = ({ onSubmit, disabled, inProgress, resetSuccess, resetError }) => (
+const NewPasswordForm = ({ onSubmit, disabled, inProgress, resetSuccess, resetError, minLength }) => (
   <form onSubmit={onSubmit}>
     <Translate
       component="p"
@@ -17,6 +17,10 @@ const NewPasswordForm = ({ onSubmit, disabled, inProgress, resetSuccess, resetEr
         type="password"
         className="standard-input"
         size="20"
+        required
+        pattern={".{"+minLength+",}"}
+        title={minLength+" characters minimum"}
+        autoComplete="new-password"
       />
     </label>
     <label>
@@ -28,6 +32,10 @@ const NewPasswordForm = ({ onSubmit, disabled, inProgress, resetSuccess, resetEr
         type="password"
         className="standard-input"
         size="20"
+        required
+        pattern={".{"+minLength+",}"}
+        title={minLength+" characters minimum"}
+        autoComplete="new-password"
       />
     </label>
     <p>
@@ -46,21 +54,20 @@ const NewPasswordForm = ({ onSubmit, disabled, inProgress, resetSuccess, resetEr
         <i className="fa fa-check-circle form-help success" />}
 
       {resetError &&
-        <Translate
-          className="form-help error"
-          component="small"
-          content="resetPassword.resetError"
-        />}
+        <small className="form-help error">
+          {resetError}
+        </small>}
     </p>
   </form>
-  );
+);
 
 NewPasswordForm.propTypes = {
   disabled: PropTypes.bool,
   inProgress: PropTypes.bool,
   onSubmit: PropTypes.func,
   resetError: PropTypes.string,
-  resetSuccess: PropTypes.bool
+  resetSuccess: PropTypes.bool,
+  minLength: PropTypes.number
 };
 
 NewPasswordForm.defaultProps = {
@@ -68,7 +75,8 @@ NewPasswordForm.defaultProps = {
   inProgress: false,
   onSubmit: () => {},
   resetError: null,
-  resetSuccess: null
+  resetSuccess: null,
+  minLength: 8
 };
 
 export default NewPasswordForm;
\ No newline at end of file
diff --git a/app/pages/reset-password/new-password-form.spec.js b/app/pages/reset-password/new-password-form.spec.js
index 8f1e1c29..c68cd64f 100644
--- a/app/pages/reset-password/new-password-form.spec.js
+++ b/app/pages/reset-password/new-password-form.spec.js
@@ -8,7 +8,6 @@ describe('NewPasswordForm', function () {
   let wrapper;
   const newPasswordSpy = sinon.spy();
 
-
   before(function () {
     wrapper = shallow(<NewPasswordForm onSubmit={newPasswordSpy} />);
   });
@@ -36,9 +35,11 @@ describe('NewPasswordForm', function () {
     assert.equal(wrapper.find('i.form-help.success').length, 0);
   });
 
-  it('conditionally shows and hides a reset password error message', function () {
-    wrapper.setProps({ resetError: 'test error message' });
-    assert.equal(wrapper.find('Translate').last().prop('content'), 'resetPassword.resetError');
+  it('conditionally shows and hides an error message', function () {
+    let expectedMsg = "Password is too short (minimum is 8 characters)";
+    wrapper.setProps({ resetError: expectedMsg });
+    let renderedText = wrapper.find('small.form-help.error').text()
+    assert.equal(renderedText, expectedMsg);
     wrapper.setProps({ resetError: null });
     assert.equal(wrapper.find('Translate').length, 3);
   });
diff --git a/app/pages/reset-password/reset-password.jsx b/app/pages/reset-password/reset-password.jsx
index 5819590d..f9dc072e 100644
--- a/app/pages/reset-password/reset-password.jsx
+++ b/app/pages/reset-password/reset-password.jsx
@@ -64,18 +64,35 @@ class ResetPasswordPage extends React.Component {
     const token = this.props.location.query.reset_password_token;
     const password = event.target[0].value;
     const confirmation = event.target[1].value;
+    const passwordMatch = password === confirmation
+    if (!passwordMatch) {
+      this.setState({
+        inProgress: false,
+        resetError: <Translate
+          component="p"
+          content="resetPassword.passwordsDoNotMatch"
+        />
+      });
+      return;
+    }
 
     auth.resetPassword({ password, confirmation, token })
       .then(() => {
         this.setState({
           resetSuccess: true,
-          inProgress: false
         });
         alert(resolve => <LoginDialog onSuccess={resolve} />);
         this.context.router.push('/projects');
       })
       .catch((error) => {
-        this.setState({ resetError: error });
+        this.setState({
+          resetError: error.message
+        });
+      })
+      .then(() => {
+        this.setState({
+          inProgress: false
+        });
       });
   }
 
@@ -101,7 +118,7 @@ class ResetPasswordPage extends React.Component {
         {this.props.location.query && this.props.location.query.reset_password_token && !this.state.resetSuccess &&
           <NewPasswordForm
             onSubmit={this.handlePasswordResetSubmit}
-            disabled={this.state.resetError || this.state.resetSuccess}
+            disabled={this.state.resetSuccess}
             inProgress={this.state.inProgress}
             resetSuccess={this.state.resetSuccess}
             resetError={this.state.resetError}
diff --git a/app/talk/board.cjsx b/app/talk/board.cjsx
index 28613865..8d2d7786 100644
--- a/app/talk/board.cjsx
+++ b/app/talk/board.cjsx
@@ -3,6 +3,8 @@ PropTypes = require 'prop-types'
 createReactClass = require 'create-react-class'
 ReactDOM = require 'react-dom'
 {Link} = require 'react-router'
+{ Helmet } = require 'react-helmet'
+counterpart = require 'counterpart'
 DiscussionPreview = require './discussion-preview'
 apiClient = require 'panoptes-client/lib/api-client'
 talkClient = require 'panoptes-client/lib/talk-client'
@@ -203,6 +205,7 @@ module.exports = createReactClass
     {board} = @state
 
     <div className="talk-board">
+      <Helmet title="#{board?.title} » #{counterpart 'projectTalk.title'}" />
       <h1 className="talk-page-header">{board?.title}</h1>
       <p>{board?.description}</p>
       <FollowBoard user={@props.user} board={board} />
diff --git a/app/talk/discussion.cjsx b/app/talk/discussion.cjsx
index a4becc04..cb0ffc8d 100644
--- a/app/talk/discussion.cjsx
+++ b/app/talk/discussion.cjsx
@@ -11,6 +11,8 @@ SingleSubmitButton = require '../components/single-submit-button'
 upvotedByCurrentUser = require './lib/upvoted-by-current-user'
 Moderation = require './lib/moderation'
 {Link} = require 'react-router'
+{ Helmet } = require 'react-helmet'
+counterpart = require 'counterpart'
 talkConfig = require './config'
 SignInPrompt = require '../partials/sign-in-prompt'
 alert = require('../lib/alert').default
@@ -305,6 +307,7 @@ module.exports = createReactClass
     {discussion} = @state
 
     <div className="talk-discussion">
+      <Helmet title="#{discussion?.title} » #{counterpart 'projectTalk.title'}" />
       {if not @state.editingTitle
         <h1 className="talk-page-header">
           {discussion?.title}


Test patch:
diff --git a/test/fake-api-client.js b/test/fake-api-client.js
new file mode 100644
index 00000000..4787f6c1
--- /dev/null
+++ b/test/fake-api-client.js
@@ -0,0 +1,50 @@
+export class FakeResource {
+  constructor(resourceType, attributes) {
+    this.resourceType = resourceType;
+    this.attributes = attributes;
+  }
+
+  save() {
+    let saved = this.resourceType.save(this.attributes);
+
+    if (saved) {
+      return Promise.resolve(this);
+    }
+    else {
+      return new Promise((resolve, reject) => { reject(new Error("Save failed!")); });
+    }
+  }
+}
+
+export class FakeResourceType {
+  constructor(apiClient, type) {
+    this.apiClient = apiClient;
+    this.type = type;
+  }
+
+  create(attributes) {
+    return new FakeResource(this, attributes);
+  }
+
+  save(attributes) { this.apiClient.save(this.type, attributes)}
+}
+
+export class FakeApiClient {
+  constructor(opts={}) {
+    this.saves = [];
+    this.canSave = opts['canSave'] || (() => { return true; });
+  }
+
+  type(resourceType) {
+    return new FakeResourceType(this, resourceType);
+  }
+
+  save(type, attributes) {
+    if (this.canSave(type, attributes)) {
+      this.saves.push({type: type, attributes: attributes});
+      return true;
+    } else {
+      return false;
+    }
+  }
+}
diff --git a/test/fake-local-storage.js b/test/fake-local-storage.js
new file mode 100644
index 00000000..7773117b
--- /dev/null
+++ b/test/fake-local-storage.js
@@ -0,0 +1,32 @@
+class FakeLocalStorage {
+  constructor() {
+    this.storage = {};
+  }
+
+  getItem(key) {
+    if (key in this.storage) {
+      return this.storage[key];
+    } else {
+      return null;
+    }
+  }
+
+  setItem(key, value) {
+    this.storage[key] = value.toString();
+  }
+
+  removeItem(key) {
+    delete this.storage[key];
+  }
+
+  get length() {
+    return Object.keys(this.storage).length;
+  }
+
+  key(i) {
+    let keys = Object.keys(this.storage);
+    return keys[i] || null;
+  }
+}
+
+export default FakeLocalStorage;
diff --git a/test/mocha.opts b/test/mocha.opts
index eb63a131..0f7b7cf6 100644
--- a/test/mocha.opts
+++ b/test/mocha.opts
@@ -1,1 +1,1 @@
- --compilers js:babel-core/register,coffee:coffee-script/register,cjsx:coffee-react/register
+ --require babel-core/register --require coffee-script/register --require coffee-react/register