From e6d7d1a9367a02c5a5ab3809c66b4488bd255bdb Mon Sep 17 00:00:00 2001 From: roschaefer Date: Thu, 19 Sep 2019 21:39:25 +0200 Subject: [PATCH 01/10] Fix #1650 --- webapp/graphql/User.js | 8 ++++++ webapp/locales/de.json | 6 +++++ webapp/locales/en.json | 6 +++++ webapp/pages/settings/index.vue | 46 ++++++++++++++++++++++++++++----- 4 files changed, 59 insertions(+), 7 deletions(-) diff --git a/webapp/graphql/User.js b/webapp/graphql/User.js index 27b3785ae..dbc1997af 100644 --- a/webapp/graphql/User.js +++ b/webapp/graphql/User.js @@ -134,3 +134,11 @@ export const unfollowUserMutation = i18n => { } ` } + +export const checkSlugAvailableQuery = gql` + query($slug: String!) { + User(slug: $slug) { + slug + } + } +` diff --git a/webapp/locales/de.json b/webapp/locales/de.json index 5b83821f5..496d3f21b 100644 --- a/webapp/locales/de.json +++ b/webapp/locales/de.json @@ -151,11 +151,17 @@ "data": { "name": "Deine Daten", "labelName": "Dein Name", + "labelSlug": "Dein eindeutiger Benutzername", "namePlaceholder": "Petra Lustig", "labelCity": "Deine Stadt oder Region", "labelBio": "Über dich", "success": "Deine Daten wurden erfolgreich aktualisiert!" }, + "validation": { + "slug": { + "alreadyTaken": "Dieser Benutzername ist schon vergeben." + } + }, "security": { "name": "Sicherheit", "change-password": { diff --git a/webapp/locales/en.json b/webapp/locales/en.json index 39220d318..660a2b1ee 100644 --- a/webapp/locales/en.json +++ b/webapp/locales/en.json @@ -152,11 +152,17 @@ "data": { "name": "Your data", "labelName": "Your Name", + "labelSlug": "Your unique user name", "namePlaceholder": "Femanon Funny", "labelCity": "Your City or Region", "labelBio": "About You", "success": "Your data was successfully updated!" }, + "validation": { + "slug": { + "alreadyTaken": "This user name is already taken." + } + }, "security": { "name": "Security", "change-password": { diff --git a/webapp/pages/settings/index.vue b/webapp/pages/settings/index.vue index 5c99f4b8b..d79c00e2f 100644 --- a/webapp/pages/settings/index.vue +++ b/webapp/pages/settings/index.vue @@ -8,6 +8,16 @@ :label="$t('settings.data.labelName')" :placeholder="$t('settings.data.namePlaceholder')" /> + + + {{ $t('settings.validation.slug.alreadyTaken') }} + @@ -42,6 +59,8 @@ import gql from 'graphql-tag' import { mapGetters, mapMutations } from 'vuex' import { CancelToken } from 'axios' +import { checkSlugAvailableQuery } from '~/graphql/User.js' +import { debounce } from 'lodash' let timeout const mapboxToken = process.env.MAPBOX_TOKEN @@ -60,9 +79,10 @@ const query = gql` */ const mutation = gql` - mutation($id: ID!, $name: String, $locationName: String, $about: String) { - UpdateUser(id: $id, name: $name, locationName: $locationName, about: $about) { + mutation($id: ID!, $slug: String, $name: String, $locationName: String, $about: String) { + UpdateUser(id: $id, slug: $slug, name: $name, locationName: $locationName, about: $about) { id + slug name locationName about @@ -78,6 +98,7 @@ export default { loadingData: false, loadingGeo: false, formData: {}, + slugAvailable: true, } }, computed: { @@ -86,21 +107,30 @@ export default { }), form: { get: function() { - const { name, locationName, about } = this.currentUser - return { name, locationName, about } + const { name, slug, locationName, about } = this.currentUser + return { name, slug, locationName, about } }, set: function(formData) { this.formData = formData }, }, }, + created() { + this.validateSlug = debounce(async () => { + const variables = { slug: this.formData.slug } + const { + data: { User }, + } = await this.$apollo.query({ query: checkSlugAvailableQuery, variables }) + this.slugAvailable = User && !User[0] + }, 500) + }, methods: { ...mapMutations({ setCurrentUser: 'auth/SET_USER', }), async submit() { this.loadingData = true - const { name, about } = this.formData + const { name, slug, about } = this.formData let { locationName } = this.formData || this.currentUser locationName = locationName && (locationName['label'] || locationName) try { @@ -109,14 +139,16 @@ export default { variables: { id: this.currentUser.id, name, + slug, locationName, about, }, update: (store, { data: { UpdateUser } }) => { - const { name, locationName, about } = UpdateUser + const { name, slug, locationName, about } = UpdateUser this.setCurrentUser({ ...this.currentUser, name, + slug, locationName, about, }) From efe9c96edb6eb2be963b0e92a6f50d3055da9962 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Thu, 19 Sep 2019 23:26:37 +0200 Subject: [PATCH 02/10] Obviously your own slug does not matter --- webapp/pages/settings/index.vue | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/webapp/pages/settings/index.vue b/webapp/pages/settings/index.vue index d79c00e2f..4cb94f313 100644 --- a/webapp/pages/settings/index.vue +++ b/webapp/pages/settings/index.vue @@ -121,7 +121,8 @@ export default { const { data: { User }, } = await this.$apollo.query({ query: checkSlugAvailableQuery, variables }) - this.slugAvailable = User && !User[0] + const existingSlug = User && User[0] && User[0].slug + this.slugAvailable = !existingSlug || existingSlug === this.currentUser.slug }, 500) }, methods: { From be6c4a6f7ca11f3d509f9551e670aa0ce3010317 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 00:42:09 +0200 Subject: [PATCH 03/10] Refactor to use FormSchema That way we can re-use the code for slug validation in the CreateUserAccount component --- webapp/components/utils/UniqueSlugForm.js | 30 ++++++ webapp/pages/settings/index.vue | 121 ++++++++++------------ 2 files changed, 86 insertions(+), 65 deletions(-) create mode 100644 webapp/components/utils/UniqueSlugForm.js diff --git a/webapp/components/utils/UniqueSlugForm.js b/webapp/components/utils/UniqueSlugForm.js new file mode 100644 index 000000000..935ef8f7e --- /dev/null +++ b/webapp/components/utils/UniqueSlugForm.js @@ -0,0 +1,30 @@ +import { debounce } from 'lodash' +import { checkSlugAvailableQuery } from '~/graphql/User.js' + +export default function UniqueSlugForm({ translate, apollo, currentUser }) { + return { + formSchema: { + slug: [ + { + asyncValidator(rule, value, callback) { + debounce(() => { + const variables = { slug: value } + apollo.query({ query: checkSlugAvailableQuery, variables }).then(response => { + const { + data: { User }, + } = response + const existingSlug = User && User[0] && User[0].slug + const available = !existingSlug || existingSlug === currentUser.slug + if (!available) { + callback(new Error(translate('settings.validation.slug.alreadyTaken'))) + } else { + callback() + } + }) + }, 500)() + }, + }, + ], + }, + } +} diff --git a/webapp/pages/settings/index.vue b/webapp/pages/settings/index.vue index 4cb94f313..d32d9a91b 100644 --- a/webapp/pages/settings/index.vue +++ b/webapp/pages/settings/index.vue @@ -1,56 +1,49 @@ @@ -59,8 +52,7 @@ import gql from 'graphql-tag' import { mapGetters, mapMutations } from 'vuex' import { CancelToken } from 'axios' -import { checkSlugAvailableQuery } from '~/graphql/User.js' -import { debounce } from 'lodash' +import UniqueSlugForm from '~/components/utils/UniqueSlugForm' let timeout const mapboxToken = process.env.MAPBOX_TOKEN @@ -98,13 +90,22 @@ export default { loadingData: false, loadingGeo: false, formData: {}, - slugAvailable: true, } }, computed: { ...mapGetters({ currentUser: 'auth/user', }), + formSchema() { + const uniqueSlugForm = UniqueSlugForm({ + apollo: this.$apollo, + currentUser: this.currentUser, + translate: this.$t, + }) + return { + ...uniqueSlugForm.formSchema, + } + }, form: { get: function() { const { name, slug, locationName, about } = this.currentUser @@ -115,16 +116,6 @@ export default { }, }, }, - created() { - this.validateSlug = debounce(async () => { - const variables = { slug: this.formData.slug } - const { - data: { User }, - } = await this.$apollo.query({ query: checkSlugAvailableQuery, variables }) - const existingSlug = User && User[0] && User[0].slug - this.slugAvailable = !existingSlug || existingSlug === this.currentUser.slug - }, 500) - }, methods: { ...mapMutations({ setCurrentUser: 'auth/SET_USER', From b4ffa1351707249c18fb9d21955ec96076b30ce0 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 01:46:06 +0200 Subject: [PATCH 04/10] Fix webapp tests by disabling the slug validations --- webapp/pages/settings/index.spec.js | 33 ++++++++++++++++++----------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/webapp/pages/settings/index.spec.js b/webapp/pages/settings/index.spec.js index f0eff0641..fcca4cbc3 100644 --- a/webapp/pages/settings/index.spec.js +++ b/webapp/pages/settings/index.spec.js @@ -24,6 +24,7 @@ describe('index.vue', () => { data: { UpdateUser: { id: 'u1', + slug: 'peter', name: 'Peter', locationName: 'Berlin', about: 'Smth', @@ -37,34 +38,42 @@ describe('index.vue', () => { }, } getters = { - 'auth/user': () => { - return {} - }, + 'auth/user': () => ({}), } }) describe('mount', () => { + let options const Wrapper = () => { store = new Vuex.Store({ getters, }) - return mount(index, { store, mocks, localVue }) + return mount(index, { store, mocks, localVue, ...options }) } + beforeEach(() => { + options = {} + }) + it('renders', () => { expect(Wrapper().contains('div')).toBe(true) }) - describe('given a new username and hitting submit', () => { - it('calls updateUser mutation', () => { - const wrapper = Wrapper() - const input = wrapper.find('#name') - const submitForm = wrapper.find('.ds-form') + describe('given we bypass the slug validations', () => { + beforeEach(() => { + // I gave up after 3 hours, feel free to remove the line below + options = { ...options, computed: { formSchema: () => ({}) } } + }) - input.setValue('Peter') - submitForm.trigger('submit') + describe('given a new username and hitting submit', () => { + it('calls updateUser mutation', () => { + const wrapper = Wrapper() - expect(mocks.$apollo.mutate).toHaveBeenCalled() + wrapper.find('#name').setValue('Peter') + wrapper.find('.ds-form').trigger('submit') + + expect(mocks.$apollo.mutate).toHaveBeenCalled() + }) }) }) }) From 4fbabc879fc17b9c00cfc7316207448b1429f913 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 20:02:51 +0200 Subject: [PATCH 05/10] Add tests for User validations --- backend/src/models/User.js | 2 +- backend/src/models/User.spec.js | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/backend/src/models/User.js b/backend/src/models/User.js index 72cef4093..4bab080ca 100644 --- a/backend/src/models/User.js +++ b/backend/src/models/User.js @@ -4,7 +4,7 @@ module.exports = { id: { type: 'string', primary: true, default: uuid }, // TODO: should be type: 'uuid' but simplified for our tests actorId: { type: 'string', allow: [null] }, name: { type: 'string', disallow: [null], min: 3 }, - slug: 'string', + slug: { type: 'string', regex: /^[a-z0-9_-]+$/, lowercase: true }, encryptedPassword: 'string', avatar: { type: 'string', allow: [null] }, coverImg: { type: 'string', allow: [null] }, diff --git a/backend/src/models/User.spec.js b/backend/src/models/User.spec.js index e00136970..5c8067413 100644 --- a/backend/src/models/User.spec.js +++ b/backend/src/models/User.spec.js @@ -18,3 +18,44 @@ describe('role', () => { ) }) }) + +describe('slug', () => { + it('normalizes to lowercase letters', async () => { + const user = await instance.create('User', { slug: 'Matt' }) + await expect(user.toJson()).resolves.toEqual( + expect.objectContaining({ + slug: 'matt', + }), + ) + }) + + describe('characters', () => { + const createUser = attrs => { + return instance.create('User', attrs).then(user => user.toJson()) + } + + it('-', async () => { + await expect(createUser({ slug: 'matt-rider' })).resolves.toMatchObject({ + slug: 'matt-rider', + }) + }) + + it('_', async () => { + await expect(createUser({ slug: 'matt_rider' })).resolves.toMatchObject({ + slug: 'matt_rider', + }) + }) + + it(' ', async () => { + await expect(createUser({ slug: 'matt rider' })).rejects.toThrow( + /fails to match the required pattern/, + ) + }) + + it('ä', async () => { + await expect(createUser({ slug: 'mätt' })).rejects.toThrow( + /fails to match the required pattern/, + ) + }) + }) +}) From e8f47cb004cb89966917790360bb99f102e0d9fd Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 20:24:04 +0200 Subject: [PATCH 06/10] Frontend validations for regex --- webapp/components/utils/UniqueSlugForm.js | 1 + webapp/locales/de.json | 1 + webapp/locales/en.json | 1 + 3 files changed, 3 insertions(+) diff --git a/webapp/components/utils/UniqueSlugForm.js b/webapp/components/utils/UniqueSlugForm.js index 935ef8f7e..c254904d0 100644 --- a/webapp/components/utils/UniqueSlugForm.js +++ b/webapp/components/utils/UniqueSlugForm.js @@ -5,6 +5,7 @@ export default function UniqueSlugForm({ translate, apollo, currentUser }) { return { formSchema: { slug: [ + {type: "string", required: true, pattern: /^[a-z0-9_-]+$/, message: translate('settings.validation.slug.regex') }, { asyncValidator(rule, value, callback) { debounce(() => { diff --git a/webapp/locales/de.json b/webapp/locales/de.json index 496d3f21b..a784cd5f7 100644 --- a/webapp/locales/de.json +++ b/webapp/locales/de.json @@ -159,6 +159,7 @@ }, "validation": { "slug": { + "regex": "Es sind nur Kleinbuchstaben, Zahlen, Unterstriche oder Bindestriche erlaubt.", "alreadyTaken": "Dieser Benutzername ist schon vergeben." } }, diff --git a/webapp/locales/en.json b/webapp/locales/en.json index 660a2b1ee..502aedd67 100644 --- a/webapp/locales/en.json +++ b/webapp/locales/en.json @@ -160,6 +160,7 @@ }, "validation": { "slug": { + "regex": "Allowed characters are only lowercase letters, numbers, underscores and hyphens.", "alreadyTaken": "This user name is already taken." } }, From c378505293edda72139e4771e939571f59b8329c Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 20:30:57 +0200 Subject: [PATCH 07/10] Add one test case for update user form --- webapp/components/utils/UniqueSlugForm.js | 7 +++++- webapp/pages/settings/index.spec.js | 29 +++++++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/webapp/components/utils/UniqueSlugForm.js b/webapp/components/utils/UniqueSlugForm.js index c254904d0..c363fa608 100644 --- a/webapp/components/utils/UniqueSlugForm.js +++ b/webapp/components/utils/UniqueSlugForm.js @@ -5,7 +5,12 @@ export default function UniqueSlugForm({ translate, apollo, currentUser }) { return { formSchema: { slug: [ - {type: "string", required: true, pattern: /^[a-z0-9_-]+$/, message: translate('settings.validation.slug.regex') }, + { + type: 'string', + required: true, + pattern: /^[a-z0-9_-]+$/, + message: translate('settings.validation.slug.regex'), + }, { asyncValidator(rule, value, callback) { debounce(() => { diff --git a/webapp/pages/settings/index.spec.js b/webapp/pages/settings/index.spec.js index fcca4cbc3..1040f2ad0 100644 --- a/webapp/pages/settings/index.spec.js +++ b/webapp/pages/settings/index.spec.js @@ -59,9 +59,34 @@ describe('index.vue', () => { expect(Wrapper().contains('div')).toBe(true) }) - describe('given we bypass the slug validations', () => { + describe('given form validation errors', () => { + beforeEach(() => { + options = { + ...options, + computed: { + formSchema: () => ({ + slug: [ + (_rule, _value, callback) => { + callback(new Error('Ouch!')) + }, + ], + }), + }, + } + }) + + it('cannot call updateUser mutation', () => { + const wrapper = Wrapper() + + wrapper.find('#name').setValue('Peter') + wrapper.find('.ds-form').trigger('submit') + + expect(mocks.$apollo.mutate).not.toHaveBeenCalled() + }) + }) + + describe('no form validation errors', () => { beforeEach(() => { - // I gave up after 3 hours, feel free to remove the line below options = { ...options, computed: { formSchema: () => ({}) } } }) From b58b4dca4f2c7d2b068c38026bcf8fe234da46ef Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 22:37:38 +0200 Subject: [PATCH 08/10] Write a test for UniqueSlugForm --- .../components/utils/UniqueSlugForm.spec.js | 80 +++++++++++++++++++ webapp/package.json | 1 + webapp/yarn.lock | 5 ++ 3 files changed, 86 insertions(+) create mode 100644 webapp/components/utils/UniqueSlugForm.spec.js diff --git a/webapp/components/utils/UniqueSlugForm.spec.js b/webapp/components/utils/UniqueSlugForm.spec.js new file mode 100644 index 000000000..de0e3fee6 --- /dev/null +++ b/webapp/components/utils/UniqueSlugForm.spec.js @@ -0,0 +1,80 @@ +import UniqueSlugForm from './UniqueSlugForm' +import Schema from 'async-validator' + +let translate +let apollo +let currentUser + +beforeEach(() => { + translate = jest.fn(() => 'Validation error') + apollo = { + query: jest.fn().mockResolvedValue({ data: { User: [] } }), + } + currentUser = null +}) + +describe('UniqueSlugForm', () => { + let validate = object => { + const { formSchema } = UniqueSlugForm({ translate, apollo, currentUser }) + const validator = new Schema(formSchema) + return validator.validate(object, { suppressWarning: true }).catch(({ errors }) => { + throw new Error(errors[0].message) + }) + } + + describe('regex', () => { + describe('non URL-safe characters, e.g. whitespaces', () => { + it('rejects', async () => { + await expect(validate({ slug: 'uh oh' })).rejects.toThrow('Validation error') + }) + }) + + describe('alphanumeric, hyphens or underscores', () => { + it('validates', async () => { + await expect(validate({ slug: '_all-right_' })).resolves.toBeUndefined() + }) + }) + }) + + describe('given a currentUser with a slug', () => { + beforeEach(() => { + currentUser = { slug: 'current-user' } + }) + + describe('backend returns no user for given slug', () => { + beforeEach(() => { + apollo.query.mockResolvedValue({ + data: { User: [] }, + }) + }) + + it('validates', async () => { + await expect(validate({ slug: 'slug' })).resolves.toBeUndefined() + }) + }) + + describe('backend returns user', () => { + let slug + beforeEach(() => { + slug = 'already-taken' + apollo.query.mockResolvedValue({ + data: { User: [{ slug: 'already-taken' }] }, + }) + }) + + it('rejects', async () => { + await expect(validate({ slug: 'uh oh' })).rejects.toThrow('Validation error') + }) + + describe('but it is the current user', () => { + beforeEach(() => { + currentUser = { slug: 'already-taken' } + }) + + it('validates', async () => { + await expect(validate({ slug })).resolves.toBeUndefined() + }) + }) + }) + }) +}) diff --git a/webapp/package.json b/webapp/package.json index 598412568..f18556e99 100644 --- a/webapp/package.json +++ b/webapp/package.json @@ -97,6 +97,7 @@ "@vue/eslint-config-prettier": "~5.0.0", "@vue/server-test-utils": "~1.0.0-beta.29", "@vue/test-utils": "~1.0.0-beta.29", + "async-validator": "^3.1.0", "babel-core": "~7.0.0-bridge.0", "babel-eslint": "~10.0.3", "babel-jest": "~24.9.0", diff --git a/webapp/yarn.lock b/webapp/yarn.lock index 6fdcc9134..a86ae8527 100644 --- a/webapp/yarn.lock +++ b/webapp/yarn.lock @@ -3649,6 +3649,11 @@ async-retry@^1.2.1: dependencies: retry "0.12.0" +async-validator@^3.1.0: + version "3.1.0" + resolved "https://registry.yarnpkg.com/async-validator/-/async-validator-3.1.0.tgz#447db5eb003cbb47e650f040037a29fc3881ce92" + integrity sha512-XyAHGwtpx3Y3aHIOaGXXFo4tiulnrh+mXBU9INxig6Q8rtmtmBxDuCxb60j7EIGbAsQg9cxfJ2jrUZ+fIqEnBQ== + async@^2.1.4: version "2.6.2" resolved "https://registry.yarnpkg.com/async/-/async-2.6.2.tgz#18330ea7e6e313887f5d2f2a904bac6fe4dd5381" From b7d70bbe906aa288ea9058e095fc008811e26b3f Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 20 Sep 2019 23:21:57 +0200 Subject: [PATCH 09/10] Fix cypress tests --- cypress/integration/common/settings.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cypress/integration/common/settings.js b/cypress/integration/common/settings.js index b32924f6a..563d6a733 100644 --- a/cypress/integration/common/settings.js +++ b/cypress/integration/common/settings.js @@ -18,6 +18,8 @@ When('I save {string} as my new name', name => { cy.get('[type=submit]') .click() .not('[disabled]') + cy.get('.iziToast-message') + .should('contain', 'Your data was successfully updated') }) When('I save {string} as my location', location => { @@ -28,6 +30,8 @@ When('I save {string} as my location', location => { cy.get('[type=submit]') .click() .not('[disabled]') + cy.get('.iziToast-message') + .should('contain', 'Your data was successfully updated') myLocation = location }) @@ -38,6 +42,8 @@ When('I have the following self-description:', text => { cy.get('[type=submit]') .click() .not('[disabled]') + cy.get('.iziToast-message') + .should('contain', 'Your data was successfully updated') aboutMeText = text }) From 77d24b5a7158d64103a3d0623d1f883a534abc97 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Mon, 23 Sep 2019 00:09:08 +0200 Subject: [PATCH 10/10] Add backend test for @mattwr18 This was already covered by `slugifyMiddleware.spec.js`. I do agree that this is a better place for it. Also we're not testing GraphQL requests here but the factories. As we're testing the existence of unique constraints, I think it won't matter if we use factories or actual GraphQL requests. --- backend/src/models/User.spec.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/backend/src/models/User.spec.js b/backend/src/models/User.spec.js index 5c8067413..7c4a26c55 100644 --- a/backend/src/models/User.spec.js +++ b/backend/src/models/User.spec.js @@ -29,6 +29,29 @@ describe('slug', () => { ) }) + it('must be unique', async done => { + await instance.create('User', { slug: 'Matt' }) + try { + await expect(instance.create('User', { slug: 'Matt' })).rejects.toThrow('already exists') + done() + } catch (error) { + throw new Error(` + ${error} + + Probably your database has no unique constraints! + + To see all constraints go to http://localhost:7474/browser/ and + paste the following: + \`\`\` + CALL db.constraints(); + \`\`\` + + Learn how to setup the database here: + https://docs.human-connection.org/human-connection/neo4j + `) + } + }) + describe('characters', () => { const createUser = attrs => { return instance.create('User', attrs).then(user => user.toJson())