From e7ab9f79a93fb4620a1ab04a6db4bd88e02f4bdf Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Sat, 20 Jul 2019 14:04:08 +0100 Subject: [PATCH 1/3] Add endpoint to remove learned words --- services/spelling/app.js | 1 + .../spelling/app/js/LearnedWordsManager.js | 19 ++++++- .../spelling/app/js/SpellingAPIController.js | 13 +++++ .../spelling/app/js/SpellingAPIManager.js | 14 +++++ .../spelling/test/acceptance/js/LearnTest.js | 29 ++++++++++ .../test/unit/js/LearnedWordsManagerTests.js | 30 ++++++++++- .../test/unit/js/SpellingAPIManagerTests.js | 53 +++++++++++++++++++ 7 files changed, 157 insertions(+), 2 deletions(-) diff --git a/services/spelling/app.js b/services/spelling/app.js index 30024dc230..cf2065f902 100644 --- a/services/spelling/app.js +++ b/services/spelling/app.js @@ -30,6 +30,7 @@ server.del('/user/:user_id', SpellingAPIController.deleteDic) server.get('/user/:user_id', SpellingAPIController.getDic) server.post('/user/:user_id/check', SpellingAPIController.check) server.post('/user/:user_id/learn', SpellingAPIController.learn) +server.post('/user/:user_id/unlearn', SpellingAPIController.unlearn) server.get('/status', (req, res) => res.send({ status: 'spelling api is up' })) server.get('/health_check', HealthCheckController.healthCheck) diff --git a/services/spelling/app/js/LearnedWordsManager.js b/services/spelling/app/js/LearnedWordsManager.js index adeb9b7dd4..bbf7651d87 100644 --- a/services/spelling/app/js/LearnedWordsManager.js +++ b/services/spelling/app/js/LearnedWordsManager.js @@ -24,6 +24,22 @@ const LearnedWordsManager = { ) }, + unlearnWord(userToken, word, callback) { + if (callback == null) { + callback = () => {} + } + mongoCache.del(userToken) + return db.spellingPreferences.update( + { + token: userToken + }, + { + $pull: { learnedWords: word } + }, + callback + ) + }, + getLearnedWords(userToken, callback) { if (callback == null) { callback = () => {} @@ -61,6 +77,7 @@ const LearnedWordsManager = { const promises = { learnWord: promisify(LearnedWordsManager.learnWord), + unlearnWord: promisify(LearnedWordsManager.unlearnWord), getLearnedWords: promisify(LearnedWordsManager.getLearnedWords), deleteUsersLearnedWords: promisify( LearnedWordsManager.deleteUsersLearnedWords @@ -70,7 +87,7 @@ const promises = { LearnedWordsManager.promises = promises module.exports = LearnedWordsManager -;['learnWord', 'getLearnedWords'].map(method => +;['learnWord', 'unlearnWord', 'getLearnedWords'].map(method => metrics.timeAsyncMethod( LearnedWordsManager, method, diff --git a/services/spelling/app/js/SpellingAPIController.js b/services/spelling/app/js/SpellingAPIController.js index 2239d997e0..e23b09da5d 100644 --- a/services/spelling/app/js/SpellingAPIController.js +++ b/services/spelling/app/js/SpellingAPIController.js @@ -49,6 +49,19 @@ module.exports = { }) }, + unlearn(req, res, next) { + metrics.inc('spelling-unlearn', 0.1) + const { token, word } = extractLearnRequestData(req) + logger.info({ token, word }, 'unlearning word') + SpellingAPIManager.unlearnWord(token, req.body, function(error) { + if (error != null) { + return next(error) + } + res.sendStatus(200) + next() + }) + }, + deleteDic(req, res, next) { const { token, word } = extractLearnRequestData(req) logger.log({ token, word }, 'deleting user dictionary') diff --git a/services/spelling/app/js/SpellingAPIManager.js b/services/spelling/app/js/SpellingAPIManager.js index 7c330eb312..3c437d929f 100644 --- a/services/spelling/app/js/SpellingAPIManager.js +++ b/services/spelling/app/js/SpellingAPIManager.js @@ -30,6 +30,20 @@ const SpellingAPIManager = { return LearnedWordsManager.learnWord(token, request.word, callback) }, + unlearnWord(token, request, callback) { + if (callback == null) { + callback = () => {} + } + if (request.word == null) { + return callback(new Error('malformed JSON')) + } + if (token == null) { + return callback(new Error('no token provided')) + } + + return LearnedWordsManager.unlearnWord(token, request.word, callback) + }, + deleteDic(token, callback) { return LearnedWordsManager.deleteUsersLearnedWords(token, callback) }, diff --git a/services/spelling/test/acceptance/js/LearnTest.js b/services/spelling/test/acceptance/js/LearnTest.js index 6f6bc21a2e..8e363b4e10 100644 --- a/services/spelling/test/acceptance/js/LearnTest.js +++ b/services/spelling/test/acceptance/js/LearnTest.js @@ -19,6 +19,14 @@ const learnWord = word => }) }) +const unlearnWord = word => + request.post({ + url: `/user/${USER_ID}/unlearn`, + body: JSON.stringify({ + word + }) + }) + const deleteDict = () => request.del({ url: `/user/${USER_ID}` @@ -51,3 +59,24 @@ describe('learning words', () => { expect(responseBody.misspellings.length).to.equals(1) }) }) + +describe('unlearning words', () => { + it('should return status 200 when posting a word successfully', async () => { + const response = await unlearnWord('anything') + expect(response.statusCode).to.equal(200) + }) + + it('should return misspellings after a word is unlearnt', async () => { + await learnWord('abv') + + const response = await checkWord(['abv']) + const responseBody = JSON.parse(response.body) + expect(responseBody.misspellings.length).to.equals(0) + + await unlearnWord('abv') + + const response2 = await checkWord(['abv']) + const responseBody2 = JSON.parse(response2.body) + expect(responseBody2.misspellings.length).to.equals(1) + }) +}) diff --git a/services/spelling/test/unit/js/LearnedWordsManagerTests.js b/services/spelling/test/unit/js/LearnedWordsManagerTests.js index 76122b1d5c..3d2324c22c 100644 --- a/services/spelling/test/unit/js/LearnedWordsManagerTests.js +++ b/services/spelling/test/unit/js/LearnedWordsManagerTests.js @@ -24,7 +24,7 @@ describe('LearnedWordsManager', function() { this.callback = sinon.stub() this.db = { spellingPreferences: { - update: sinon.stub().callsArg(3) + update: sinon.stub().yields() } } this.cache = { @@ -80,6 +80,34 @@ describe('LearnedWordsManager', function() { }) }) + describe('unlearnWord', function() { + beforeEach(function() { + this.word = 'instanton' + return this.LearnedWordsManager.unlearnWord( + this.token, + this.word, + this.callback + ) + }) + + it('should remove the word from the word list in the database', function() { + return expect( + this.db.spellingPreferences.update.calledWith( + { + token: this.token + }, + { + $pull: { learnedWords: this.word } + } + ) + ).to.equal(true) + }) + + return it('should call the callback', function() { + return expect(this.callback.called).to.equal(true) + }) + }) + describe('getLearnedWords', function() { beforeEach(function() { this.wordList = ['apples', 'bananas', 'pears'] diff --git a/services/spelling/test/unit/js/SpellingAPIManagerTests.js b/services/spelling/test/unit/js/SpellingAPIManagerTests.js index 5baecbc8b0..f2e1bc55cc 100644 --- a/services/spelling/test/unit/js/SpellingAPIManagerTests.js +++ b/services/spelling/test/unit/js/SpellingAPIManagerTests.js @@ -21,6 +21,7 @@ describe('SpellingAPIManager', function() { this.LearnedWordsManager = { getLearnedWords: sinon.stub().callsArgWith(1, null, this.learnedWords), learnWord: sinon.stub().callsArg(2), + unlearnWord: sinon.stub().callsArg(2), promises: { getLearnedWords: sinon.stub().returns(promiseStub(this.learnedWords)) } @@ -229,4 +230,56 @@ describe('SpellingAPIManager', function() { }) }) }) + + describe('unlearnWord', function() { + describe('without a token', function() { + beforeEach(function(done) { + this.SpellingAPIManager.unlearnWord(null, { word: 'banana' }, error => { + this.error = error + done() + }) + }) + + it('should return an error', function() { + expect(this.error).to.exist + expect(this.error).to.be.instanceof(Error) + expect(this.error.message).to.equal('no token provided') + }) + }) + + describe('without a word', function() { + beforeEach(function(done) { + this.SpellingAPIManager.unlearnWord(this.token, {}, error => { + this.error = error + done() + }) + }) + + it('should return an error', function() { + expect(this.error).to.exist + expect(this.error).to.be.instanceof(Error) + expect(this.error.message).to.equal('malformed JSON') + }) + }) + + describe('with a word and a token', function() { + beforeEach(function(done) { + this.word = 'banana' + this.SpellingAPIManager.unlearnWord( + this.token, + { word: this.word }, + error => { + this.error = error + done() + } + ) + }) + + it('should call LearnedWordsManager.unlearnWord', function() { + this.LearnedWordsManager.unlearnWord + .calledWith(this.token, this.word) + .should.equal(true) + }) + }) + }) }) From 7105ee58d3fb18d7ad01598bd791300de473b734 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Mon, 22 Jul 2019 12:14:30 +0100 Subject: [PATCH 2/3] Cleanup decaffienation in `LearnedWordsManagerTests` --- .../test/unit/js/LearnedWordsManagerTests.js | 105 ++++++------------ 1 file changed, 37 insertions(+), 68 deletions(-) diff --git a/services/spelling/test/unit/js/LearnedWordsManagerTests.js b/services/spelling/test/unit/js/LearnedWordsManagerTests.js index 3d2324c22c..32ea14a18a 100644 --- a/services/spelling/test/unit/js/LearnedWordsManagerTests.js +++ b/services/spelling/test/unit/js/LearnedWordsManagerTests.js @@ -1,14 +1,3 @@ -/* eslint-disable - handle-callback-err, - no-undef -*/ -// TODO: This file was created by bulk-decaffeinate. -// Sanity-check the conversion and remove this comment. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const chai = require('chai') const { expect } = chai @@ -32,7 +21,7 @@ describe('LearnedWordsManager', function() { set: sinon.stub(), del: sinon.stub() } - return (this.LearnedWordsManager = SandboxedModule.require(modulePath, { + this.LearnedWordsManager = SandboxedModule.require(modulePath, { requires: { './DB': this.db, './MongoCache': this.cache, @@ -46,21 +35,17 @@ describe('LearnedWordsManager', function() { inc: sinon.stub() } } - })) + }) }) describe('learnWord', function() { beforeEach(function() { this.word = 'instanton' - return this.LearnedWordsManager.learnWord( - this.token, - this.word, - this.callback - ) + this.LearnedWordsManager.learnWord(this.token, this.word, this.callback) }) it('should insert the word in the word list in the database', function() { - return expect( + expect( this.db.spellingPreferences.update.calledWith( { token: this.token @@ -75,23 +60,19 @@ describe('LearnedWordsManager', function() { ).to.equal(true) }) - return it('should call the callback', function() { - return expect(this.callback.called).to.equal(true) + it('should call the callback', function() { + expect(this.callback.called).to.equal(true) }) }) describe('unlearnWord', function() { beforeEach(function() { this.word = 'instanton' - return this.LearnedWordsManager.unlearnWord( - this.token, - this.word, - this.callback - ) + this.LearnedWordsManager.unlearnWord(this.token, this.word, this.callback) }) it('should remove the word from the word list in the database', function() { - return expect( + expect( this.db.spellingPreferences.update.calledWith( { token: this.token @@ -103,8 +84,8 @@ describe('LearnedWordsManager', function() { ).to.equal(true) }) - return it('should call the callback', function() { - return expect(this.callback.called).to.equal(true) + it('should call the callback', function() { + expect(this.callback.called).to.equal(true) }) }) @@ -112,22 +93,20 @@ describe('LearnedWordsManager', function() { beforeEach(function() { this.wordList = ['apples', 'bananas', 'pears'] this.db.spellingPreferences.findOne = (conditions, callback) => { - return callback(null, { learnedWords: this.wordList }) + callback(null, { learnedWords: this.wordList }) } sinon.spy(this.db.spellingPreferences, 'findOne') - return this.LearnedWordsManager.getLearnedWords(this.token, this.callback) + this.LearnedWordsManager.getLearnedWords(this.token, this.callback) }) it('should get the word list for the given user', function() { - return expect( + expect( this.db.spellingPreferences.findOne.calledWith({ token: this.token }) ).to.equal(true) }) - return it('should return the word list in the callback', function() { - return expect(this.callback.calledWith(null, this.wordList)).to.equal( - true - ) + it('should return the word list in the callback', function() { + expect(this.callback.calledWith(null, this.wordList)).to.equal(true) }) }) @@ -136,14 +115,12 @@ describe('LearnedWordsManager', function() { this.wordList = ['apples', 'bananas', 'pears'] this.cache.get.returns(this.wordList) this.db.spellingPreferences.findOne = sinon.stub() - return this.LearnedWordsManager.getLearnedWords( - this.token, - (err, spellings) => { - this.db.spellingPreferences.findOne.called.should.equal(false) - assert.deepEqual(this.wordList, spellings) - return done() - } - ) + this.LearnedWordsManager.getLearnedWords(this.token, (err, spellings) => { + expect(err).not.to.exist + this.db.spellingPreferences.findOne.called.should.equal(false) + assert.deepEqual(this.wordList, spellings) + done() + }) }) it('should set the cache after hitting the db', function(done) { @@ -151,41 +128,33 @@ describe('LearnedWordsManager', function() { this.db.spellingPreferences.findOne = sinon .stub() .callsArgWith(1, null, { learnedWords: this.wordList }) - return this.LearnedWordsManager.getLearnedWords( - this.token, - (err, spellings) => { - this.cache.set - .calledWith(this.token, this.wordList) - .should.equal(true) - return done() - } - ) + this.LearnedWordsManager.getLearnedWords(this.token, () => { + this.cache.set.calledWith(this.token, this.wordList).should.equal(true) + done() + }) }) - return it('should break cache when update is called', function(done) { + it('should break cache when update is called', function(done) { this.word = 'instanton' - return this.LearnedWordsManager.learnWord(this.token, this.word, () => { + this.LearnedWordsManager.learnWord(this.token, this.word, () => { this.cache.del.calledWith(this.token).should.equal(true) - return done() + done() }) }) }) - return describe('deleteUsersLearnedWords', function() { + describe('deleteUsersLearnedWords', function() { beforeEach(function() { - return (this.db.spellingPreferences.remove = sinon.stub().callsArgWith(1)) + this.db.spellingPreferences.remove = sinon.stub().callsArgWith(1) }) - return it('should get the word list for the given user', function(done) { - return this.LearnedWordsManager.deleteUsersLearnedWords( - this.token, - () => { - this.db.spellingPreferences.remove - .calledWith({ token: this.token }) - .should.equal(true) - return done() - } - ) + it('should get the word list for the given user', function(done) { + this.LearnedWordsManager.deleteUsersLearnedWords(this.token, () => { + this.db.spellingPreferences.remove + .calledWith({ token: this.token }) + .should.equal(true) + done() + }) }) }) }) From 00d09fd6f5848baf4408cb5a01674903f3be8f78 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Mon, 22 Jul 2019 12:15:07 +0100 Subject: [PATCH 3/3] Return 204 instead of 200 when [un]learning words Also remove unnecessary calls to `next()` --- services/spelling/app/js/SpellingAPIController.js | 6 ++---- services/spelling/test/acceptance/js/LearnTest.js | 8 ++++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/services/spelling/app/js/SpellingAPIController.js b/services/spelling/app/js/SpellingAPIController.js index e23b09da5d..f771a7a567 100644 --- a/services/spelling/app/js/SpellingAPIController.js +++ b/services/spelling/app/js/SpellingAPIController.js @@ -44,8 +44,7 @@ module.exports = { if (error != null) { return next(error) } - res.sendStatus(200) - next() + res.sendStatus(204) }) }, @@ -57,8 +56,7 @@ module.exports = { if (error != null) { return next(error) } - res.sendStatus(200) - next() + res.sendStatus(204) }) }, diff --git a/services/spelling/test/acceptance/js/LearnTest.js b/services/spelling/test/acceptance/js/LearnTest.js index 8e363b4e10..b37efc5e80 100644 --- a/services/spelling/test/acceptance/js/LearnTest.js +++ b/services/spelling/test/acceptance/js/LearnTest.js @@ -33,9 +33,9 @@ const deleteDict = () => }) describe('learning words', () => { - it('should return status 200 when posting a word successfully', async () => { + it('should return status 204 when posting a word successfully', async () => { const response = await learnWord('abcd') - expect(response.statusCode).to.equal(200) + expect(response.statusCode).to.equal(204) }) it('should return no misspellings after a word is learnt', async () => { @@ -61,9 +61,9 @@ describe('learning words', () => { }) describe('unlearning words', () => { - it('should return status 200 when posting a word successfully', async () => { + it('should return status 204 when posting a word successfully', async () => { const response = await unlearnWord('anything') - expect(response.statusCode).to.equal(200) + expect(response.statusCode).to.equal(204) }) it('should return misspellings after a word is unlearnt', async () => {