407 change your email address #1711
Conversation
It might be that people try to register email addresses that they don't own. Then if the actual owner tries to add this email address, she should not get a unique constraint violation. Instead the email will be re-used. Is this a security issue? Because we re-use the nonce? 🤔
So, to get a direct link it's better to have one route that calls a mutation as soon as it is visited.
@alina-beck said we have most buttons left-aligned, so I went with that: Human-Connection/Human-Connection#1711 (comment) Also this uses icon `envelope` for emails. This makes sense, because we could use icon `at` for slugs.
`BELONGS_TO` means a user owns an email address. `PRIMARY_EMAIL` means a user authenticates with that email. So right now, you get a proper error message if you try to change your email back to your old email address (because you own it already). I will make sure to delete the old email so this will be no problem anymore. But maybe in the future we might have multiple email addresses per user and then it makes a big difference to use `PRIMARY_EMAIL` or `BELONGS_TO`.
In the registration resolvers, it makes sense to immediately resolve if an email address has been found (because you can re-send the registration email). In this case, we use the helper method only to trigger the `UserInputError`.
This will allow you to change back to your previous email address: The backend won't complain because of a user who owns that email address already.
| @@ -0,0 +1,4 @@ | |||
| import uuid from 'uuid/v4' | |||
| export default function generateNonce() { | |||
| return uuid().substring(0, 6) | |||
There was a problem hiding this comment.
Authored by roschaefer
Sep 28, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,4 @@
+import uuid from 'uuid/v4'
+export default function generateNonce() {
+ return uuid().substring(0, 6)@datenbrei you wanted to know how the nonce gets generated
| }) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,283 @@
+import Factory from '../../seed/factories'
+import { gql } from '../../jest/helpers'
+import { getDriver, neode as getNeode } from '../../bootstrap/neo4j'
+import createServer from '../../server'
+import { createTestClient } from 'apollo-server-testing'
+
+const factory = Factory()
+const neode = getNeode()
+
+let mutate
+let authenticatedUser
+let user
+let variables
+const driver = getDriver()
+
+beforeEach(async () => {
+ variables = {}
+})
+
+beforeAll(() => {
+ const { server } = createServer({
+ context: () => {
+ return {
+ driver,
+ neode,
+ user: authenticatedUser,
+ }
+ },
+ })
+ mutate = createTestClient(server).mutate
+})
+
+afterEach(async () => {
+ await factory.cleanDatabase()
+})
+
+describe('AddEmailAddress', () => {
+ const mutation = gql`
+ mutation($email: String!) {
+ AddEmailAddress(email: $email) {
+ email
+ verifiedAt
+ createdAt
+ }
+ }
+ `
+ beforeEach(() => {
+ variables = { ...variables, email: 'new-email@example.org' }
+ })
+
+ describe('unauthenticated', () => {
+ beforeEach(() => {
+ authenticatedUser = null
+ })
+
+ it('throws AuthorizationError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { AddEmailAddress: null },
+ errors: [{ message: 'Not Authorised!' }],
+ })
+ })
+ })
+
+ describe('authenticated', () => {
+ beforeEach(async () => {
+ user = await factory.create('User', { email: 'user@example.org' })
+ authenticatedUser = await user.toJson()
+ })
+
+ describe('email attribute is not a valid email', () => {
+ beforeEach(() => {
+ variables = { ...variables, email: 'foobar' }
+ })
+
+ it('throws UserInputError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { AddEmailAddress: null },
+ errors: [{ message: 'must be a valid email' }],
+ })
+ })
+ })
+
+ describe('email attribute is a valid email', () => {
+ it('creates a new unverified `EmailAddress` node', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: {
+ AddEmailAddress: {
+ email: 'new-email@example.org',
+ verifiedAt: null,
+ createdAt: expect.any(String),
+ },
+ },
+ errors: undefined,
+ })
+ })
+
+ it('connects `EmailAddressRequest` to the authenticated user', async () => {
+ await mutate({ mutation, variables })
+ const result = await neode.cypher(`
+ MATCH(u:User)-[:PRIMARY_EMAIL]->(:EmailAddress {email: "user@example.org"})
+ MATCH(u:User)<-[:BELONGS_TO]-(e:EmailAddressRequest {email: "new-email@example.org"})
+ RETURN e
+ `)
+ const email = neode.hydrateFirst(result, 'e', neode.model('EmailAddressRequest'))
+ await expect(email.toJson()).resolves.toMatchObject({
+ email: 'new-email@example.org',
+ nonce: expect.any(String),
+ })
+ })
+
+ describe('if another `EmailAddressRequest` node already exists with that email', () => {
+ it('throws no unique constraint violation error', async () => {
+ await factory.create('EmailAddressRequest', {
+ createdAt: '2019-09-24T14:00:01.565Z',
+ email: 'new-email@example.org',
+ })
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: {
+ AddEmailAddress: {
+ email: 'new-email@example.org',
+ verifiedAt: null,
+ },
+ },
+ errors: undefined,
+ })
+ })
+ })
+
+ describe('but if another user owns an `EmailAddress` already with that email', () => {
+ it('throws UserInputError because of unique constraints', async () => {
+ await factory.create('User', { email: 'new-email@example.org' })
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { AddEmailAddress: null },
+ errors: [{ message: 'A user account with this email already exists.' }],
+ })
+ })
+ })
+ })
+ })
+})
+
+describe('VerifyEmailAddress', () => {
+ const mutation = gql`
+ mutation($email: String!, $nonce: String!) {
+ VerifyEmailAddress(email: $email, nonce: $nonce) {
+ email
+ createdAt
+ verifiedAt
+ }
+ }
+ `
+
+ beforeEach(() => {
+ variables = { ...variables, email: 'to-be-verified@example.org', nonce: '123456' }
+ })
+
+ describe('unauthenticated', () => {
+ beforeEach(() => {
+ authenticatedUser = null
+ })
+
+ it('throws AuthorizationError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { VerifyEmailAddress: null },
+ errors: [{ message: 'Not Authorised!' }],
+ })
+ })
+ })
+
+ describe('authenticated', () => {
+ beforeEach(async () => {
+ user = await factory.create('User', { email: 'user@example.org' })
+ authenticatedUser = await user.toJson()
+ })
+
+ describe('if no unverified `EmailAddress` node exists', () => {
+ it('throws UserInputError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { VerifyEmailAddress: null },
+ errors: [{ message: 'Invalid nonce or no email address found.' }],
+ })
+ })
+ })
+
+ describe('given a `EmailAddressRequest`', () => {
+ let emailAddress
+ beforeEach(async () => {
+ emailAddress = await factory.create('EmailAddressRequest', {
+ nonce: 'abcdef',
+ verifiedAt: null,
+ createdAt: new Date().toISOString(),
+ email: 'to-be-verified@example.org',
+ })
+ })
+
+ describe('given invalid nonce', () => {
+ it('throws UserInputError', async () => {
+ variables.nonce = 'asdfgh'
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { VerifyEmailAddress: null },
+ errors: [{ message: 'Invalid nonce or no email address found.' }],
+ })
+ })
+ })
+
+ describe('given valid nonce for `EmailAddressRequest` node', () => {
+ beforeEach(() => {
+ variables = { ...variables, nonce: 'abcdef' }
+ })
+
+ describe('but the address does not belong to the authenticated user', () => {
+ it('throws UserInputError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { VerifyEmailAddress: null },
+ errors: [{ message: 'Invalid nonce or no email address found.' }],
+ })
+ })
+ })
+
+ describe('and the `EmailAddressRequest` belongs to the authenticated user', () => {
+ beforeEach(async () => {
+ await emailAddress.relateTo(user, 'belongsTo')
+ })
+
+ it('adds `verifiedAt`', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: {
+ VerifyEmailAddress: {
+ email: 'to-be-verified@example.org',
+ verifiedAt: expect.any(String),
+ createdAt: expect.any(String),
+ },
+ },
+ errors: undefined,
+ })
+ })
+
+ it('connects the new `EmailAddress` as PRIMARY', async () => {
+ await mutate({ mutation, variables })
+ const result = await neode.cypher(`
+ MATCH(u:User)-[:PRIMARY_EMAIL]->(e:EmailAddress {email: "to-be-verified@example.org"})
+ MATCH(u:User)<-[:BELONGS_TO]-(:EmailAddress {email: "user@example.org"})
+ RETURN e
+ `)
+ const email = neode.hydrateFirst(result, 'e', neode.model('EmailAddress'))
+ await expect(email.toJson()).resolves.toMatchObject({
+ email: 'to-be-verified@example.org',
+ })
+ })
+
+ it('removes previous PRIMARY relationship', async () => {
+ const cypherStatement = `
+ MATCH(u:User)-[:PRIMARY_EMAIL]->(e:EmailAddress {email: "user@example.org"})
+ RETURN e
+ `
+ let result = await neode.cypher(cypherStatement)
+ let email = neode.hydrateFirst(result, 'e', neode.model('EmailAddress'))
+ await expect(email.toJson()).resolves.toMatchObject({
+ email: 'user@example.org',
+ })
+ await mutate({ mutation, variables })
+ result = await neode.cypher(cypherStatement)
+ email = neode.hydrateFirst(result, 'e', neode.model('EmailAddress'))
+ await expect(email).toBe(false)
+ })
+
+ describe('Edge case: In the meantime someone created an `EmailAddress` node with the given email', () => {
+ beforeEach(async () => {
+ await factory.create('EmailAddress', { email: 'to-be-verified@example.org' })
+ })
+
+ it('throws UserInputError because of unique constraints', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { VerifyEmailAddress: null },
+ errors: [{ message: 'A user account with this email already exists.' }],
+ })
+ })
+ })
+ })
+ })
+ })
+ })
+})Wow, this is thoroughly tested! 👍
There was a problem hiding this comment.
Oct 2, 2019
I love it!! I guess you probably didn't Test Drive this since the implementation changed a few times??
There was a problem hiding this comment.
Oct 2, 2019
Hard to believe, but I test drove the backend 😉
For the frontend I added tests after the implementation because at the time I had hopes to find a solution with less page components and redirects. In the end I figured that solution to be the least complex though.
There was a problem hiding this comment.
| @@ -307,6 +307,7 @@ describe('Signup', () => { | |||
| it('is allowed to signup users by email', async () => { | |||
| await expect(mutate({ mutation, variables })).resolves.toMatchObject({ | |||
| data: { Signup: { email: 'someuser@example.org' } }, | |||
| errors: undefined, | |||
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -307,6 +307,7 @@ describe('Signup', () => {
it('is allowed to signup users by email', async () => {
await expect(mutate({ mutation, variables })).resolves.toMatchObject({
data: { Signup: { email: 'someuser@example.org' } },
+ errors: undefined,Is this necessary? I think toMatchObject does not need the full object defined, it should also pass for partial matches. Or do you want to explicitly make sure that errors are indeed undefined?
There was a problem hiding this comment.
Oct 2, 2019
Yes, I do. I thought it might increase readability and maybe save us from false negatives.
| @@ -351,6 +352,7 @@ describe('Signup', () => { | |||
| it('resolves with the already existing email', async () => { | |||
| await expect(mutate({ mutation, variables })).resolves.toMatchObject({ | |||
| data: { Signup: { email: 'someuser@example.org' } }, | |||
| errors: undefined, | |||
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -351,6 +352,7 @@ describe('Signup', () => {
it('resolves with the already existing email', async () => {
await expect(mutate({ mutation, variables })).resolves.toMatchObject({
data: { Signup: { email: 'someuser@example.org' } },
+ errors: undefined,same question as above ☝️
There was a problem hiding this comment.
| @@ -359,6 +361,7 @@ describe('Signup', () => { | |||
| await expect(neode.all('EmailAddress')).resolves.toHaveLength(2) | |||
| await expect(mutate({ mutation, variables })).resolves.toMatchObject({ | |||
| data: { Signup: { email: 'someuser@example.org' } }, | |||
| errors: undefined, | |||
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -359,6 +361,7 @@ describe('Signup', () => {
await expect(neode.all('EmailAddress')).resolves.toHaveLength(2)
await expect(mutate({ mutation, variables })).resolves.toMatchObject({
data: { Signup: { email: 'someuser@example.org' } },
+ errors: undefined,and again ☝️
| @@ -158,6 +158,27 @@ | |||
| "labelBio": "Über dich", | |||
| "success": "Deine Daten wurden erfolgreich aktualisiert!" | |||
| }, | |||
| "email": { | |||
| "validation": { | |||
| "same-email": "Das ist deine aktuelle E-Mail Addresse" | |||
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -157,6 +157,28 @@
"labelBio": "Über dich",
"success": "Deine Daten wurden erfolgreich aktualisiert!"
},
+ "email": {
+ "validation": {
+ "same-email": "Muss sich unterscheiden von der jetzigen E-Mail Addresse"Mir würde besser gefallen: Das ist deine aktuelle E-Mail Adresse
| @@ -159,6 +159,27 @@ | |||
| "labelBio": "About You", | |||
| "success": "Your data was successfully updated!" | |||
| }, | |||
| "email": { | |||
| "validation": { | |||
| "same-email": "This is your current email address" | |||
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -158,6 +158,28 @@
"labelBio": "About You",
"success": "Your data was successfully updated!"
},
+ "email": {
+ "validation": {
+ "same-email": "Must be different from your current E-Mail address"Different language, same idea: This is your current e-mail address 😉
What you wrote sounds good in English but not very friendly in German – to my ears, at least. And if we decide to change it I think we should change it in both languages. I'll leave it up to you! :)
| "no-email-request": "Are you certain that you requested a change of your email address?" | ||
| }, | ||
| "support": "If the problem persists, please contact us by email at" | ||
| } |
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -158,6 +158,28 @@
"labelBio": "About You",
"success": "Your data was successfully updated!"
},
+ "email": {
+ "validation": {
+ "same-email": "Must be different from your current E-Mail address"
+ },
+ "name": "Your E-Mail",
+ "labelEmail": "Change your E-Mail address",
+ "labelNewEmail": "New E-Mail Address",
+ "labelNonce": "Enter your code",
+ "success": "A new E-Mail address has been registered.",
+ "submitted": "An email to verify your address has been sent to <b>{email}</b>.",
+ "change-successful": "Your E-Mail address has been changed successfully.",
+ "verification-error": {
+ "message": "Your E-Mail could not be changed.",
+ "explanation": "This can have different causes:",
+ "reason": {
+ "invalid-nonce": "Is the confirmation code invalid?",
+ "no-email-request": "You haven't requested a change of your email address at all?",
+ "email-address-taken": "Has the email been assigned to another user in the meantime?"
+ },
+ "support": "If the problem persists, please contact us by e-mail at"I think last time we decided to go with the spelling e-mail in English, or am I mistaken @Tirokk? Would be great to have it all the same – and there are some E-Mails and emails in this file. 😜
There was a problem hiding this comment.
Oct 1, 2019
I think we didn't decide... @Tirokk asked it to be changed in your PR cause it's what he prefers, and since no one else cared enough to point it out, it's what we went with. I prefer the non-hyphenated version personally. While e-mail is still more commonly used than email, it's on the downturn, so if we wanna keep up with the times, maybe we should be cool and use email 😝
https://writingexplained.org/e-mail-or-email
There was a problem hiding this comment.
There was a problem hiding this comment.
Oct 2, 2019
In German it seems to E-Mail and nothing else: https://www.duden.de/sprachwissen/sprachratgeber/Schreibung-von-E-Mail
There was a problem hiding this comment.
Oct 2, 2019
I say just leave it, like I said, it's not something I would ask to be changed.
Sorry, I just couldn't help myself when @alina-beck said we decided... e-mail is perfectly ok, there seem to be stricter rules in German than English and as long as it's not incorrect, we can follow the stricter rules to have some continuity between the two.
There was a problem hiding this comment.
Oct 2, 2019
I really don't mind if we go with email or e-mail in English! (Personally I've only been using email so far...) But I think it would be great if we could agree on one and then stick to it in all user-facing communication.
It's not a big issue and most people won't notice, I guess, but some (language 🤓 like me) might and it's just overall smoother and cleaner. 😉
Since I don't have a strong opinion I just went with what @Tirokk suggested last time. But what do you say @mattwr18? Would email be better? Let's decide! 😄
There was a problem hiding this comment.
Oct 2, 2019
I wouldn't say it would be better, I don't think it really matters one way or the other, and I completely understand for user facing communication it's great to be consistent. It's a little funny to me cause all of our keys in the locale files we spell it email not e-mail, but those are not user facing.
I'm happy to use e-mail for the reasons I stated above ☝️
Now, we can probably freely say we decided 🤣
| <ds-form v-else v-model="form" :schema="formSchema" @submit="submit"> | ||
| <template slot-scope="{ errors }"> | ||
| <ds-card :header="$t('settings.email.name')"> | ||
| <ds-input |
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,114 @@
+<template>
+ <ds-card centered v-if="success">
+ <transition name="ds-transition-fade">
+ <sweetalert-icon icon="info" />
+ </transition>
+ <ds-text v-html="submitMessage" />
+ </ds-card>
+ <ds-form v-else v-model="form" :schema="formSchema" @submit="submit">
+ <template slot-scope="{ errors }">
+ <ds-card :header="$t('settings.email.name')">
+ <ds-input id="email" model="email" icon="at" :label="$t('settings.email.labelEmail')" />in the login e-mail input we use the envelope icon which I would prefer here as well 🙂
| } | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,114 @@
+<template>
+ <ds-card centered v-if="success">
+ <transition name="ds-transition-fade">
+ <sweetalert-icon icon="info" />
+ </transition>
+ <ds-text v-html="submitMessage" />
+ </ds-card>
+ <ds-form v-else v-model="form" :schema="formSchema" @submit="submit">
+ <template slot-scope="{ errors }">
+ <ds-card :header="$t('settings.email.name')">
+ <ds-input id="email" model="email" icon="at" :label="$t('settings.email.labelEmail')" />
+
+ <template slot="footer">
+ <ds-space class="backendErrors" v-if="backendErrors">
+ <ds-text align="center" bold color="danger">{{ backendErrors.message }}</ds-text>
+ </ds-space>
+ <ds-button class="submit-button" icon="check" :disabled="errors" type="submit" primary>
+ {{ $t('actions.save') }}
+ </ds-button>
+ </template>
+ </ds-card>
+ </template>
+ </ds-form>
+</template>
+
+<script>
+import { mapGetters } from 'vuex'
+import { AddEmailAddressMutation } from '~/graphql/EmailAddress.js'
+import { SweetalertIcon } from 'vue-sweetalert-icons'
+
+export default {
+ components: {
+ SweetalertIcon,
+ },
+ data() {
+ return {
+ backendErrors: null,
+ success: false,
+ }
+ },
+ computed: {
+ submitMessage() {
+ const { email } = this.formData
+ return this.$t('settings.email.submitted', { email })
+ },
+ ...mapGetters({
+ currentUser: 'auth/user',
+ }),
+ form: {
+ get: function() {
+ const { email } = this.currentUser
+ return { email }
+ },
+ set: function(formData) {
+ this.formData = formData
+ },
+ },
+ formSchema() {
+ const { email } = this.currentUser
+ const sameEmailValidationError = this.$t('settings.email.validation.same-email')
+ return {
+ email: [
+ { type: 'email', required: true },
+ {
+ validator(rule, value, callback, source, options) {
+ const errors = []
+ if (email === value) {
+ errors.push(sameEmailValidationError)
+ }
+ return errors
+ },
+ },
+ ],
+ }
+ },
+ },
+ methods: {
+ async submit() {
+ const { email } = this.formData
+ try {
+ await this.$apollo.mutate({
+ mutation: AddEmailAddressMutation,
+ variables: { email },
+ })
+ this.$toast.success(this.$t('settings.email.success'))
+ this.success = true
+
+ setTimeout(() => {
+ this.$router.push({
+ path: 'my-email-address/enter-nonce',
+ query: { email },
+ })
+ }, 3000)
+ } catch (err) {
+ if (err.message.includes('exists')) {
+ // We cannot use form validation errors here, the backend does not
+ // have a query to filter for email addresses. This is a privacy
+ // consideration. We could implement a dedicated query to check that
+ // but I think it's too much effort for this feature.
+ this.backendErrors = { message: this.$t('registration.signup.form.errors.email-exists') }
+ return
+ }
+ this.$toast.error(err.message)
+ }
+ },
+ },
+}
+</script>
+
+<style lang="scss" scoped>
+.submit-button {
+ float: right;why? all the other buttons (e.g. add social media, delete data, change password) are left aligned
| path: 'my-email-address/enter-nonce', | ||
| query: { email }, | ||
| }) | ||
| }, 3000) |
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,114 @@
+<template>
+ <ds-card centered v-if="success">
+ <transition name="ds-transition-fade">
+ <sweetalert-icon icon="info" />
+ </transition>
+ <ds-text v-html="submitMessage" />
+ </ds-card>
+ <ds-form v-else v-model="form" :schema="formSchema" @submit="submit">
+ <template slot-scope="{ errors }">
+ <ds-card :header="$t('settings.email.name')">
+ <ds-input id="email" model="email" icon="at" :label="$t('settings.email.labelEmail')" />
+
+ <template slot="footer">
+ <ds-space class="backendErrors" v-if="backendErrors">
+ <ds-text align="center" bold color="danger">{{ backendErrors.message }}</ds-text>
+ </ds-space>
+ <ds-button class="submit-button" icon="check" :disabled="errors" type="submit" primary>
+ {{ $t('actions.save') }}
+ </ds-button>
+ </template>
+ </ds-card>
+ </template>
+ </ds-form>
+</template>
+
+<script>
+import { mapGetters } from 'vuex'
+import { AddEmailAddressMutation } from '~/graphql/EmailAddress.js'
+import { SweetalertIcon } from 'vue-sweetalert-icons'
+
+export default {
+ components: {
+ SweetalertIcon,
+ },
+ data() {
+ return {
+ backendErrors: null,
+ success: false,
+ }
+ },
+ computed: {
+ submitMessage() {
+ const { email } = this.formData
+ return this.$t('settings.email.submitted', { email })
+ },
+ ...mapGetters({
+ currentUser: 'auth/user',
+ }),
+ form: {
+ get: function() {
+ const { email } = this.currentUser
+ return { email }
+ },
+ set: function(formData) {
+ this.formData = formData
+ },
+ },
+ formSchema() {
+ const { email } = this.currentUser
+ const sameEmailValidationError = this.$t('settings.email.validation.same-email')
+ return {
+ email: [
+ { type: 'email', required: true },
+ {
+ validator(rule, value, callback, source, options) {
+ const errors = []
+ if (email === value) {
+ errors.push(sameEmailValidationError)
+ }
+ return errors
+ },
+ },
+ ],
+ }
+ },
+ },
+ methods: {
+ async submit() {
+ const { email } = this.formData
+ try {
+ await this.$apollo.mutate({
+ mutation: AddEmailAddressMutation,
+ variables: { email },
+ })
+ this.$toast.success(this.$t('settings.email.success'))
+ this.success = true
+
+ setTimeout(() => {
+ this.$router.push({
+ path: 'my-email-address/enter-nonce',
+ query: { email },
+ })
+ }, 3000)I don't like that we are mixing different types of alerts – toasts, the error in the footer, the success block in combination with this timeout... they're all over the place! And in this case not really self-contained when the submit() method has to decide how long the alert is displayed. 😕
Do you think we could agree on just one version, make a proper component for it and then use it everywhere? 😄 Maybe not in this PR, though...
There was a problem hiding this comment.
Oct 2, 2019
Yes, totally agree. I would prefer form validation errors. I try my best to implement re-usable async form validators, e.g. the one that checks for unique slugs.
However in this case we have no backend query to check if a particular already exists. Sure - I could probably implement a backend query for frontend validation. But I thought maybe we don't want to do that? 🤔
So right now, we have to call the mutation and create the user account. If it fails, we have to check the backend error message. It would be great to get the form validation error into the <ds-form> component somehow, but I believe that's not possible right now.
There was a problem hiding this comment.
Oct 2, 2019
I also prefer form validators as it's closer to where the user's attention is, but then, as far as I know, we'd have toastrs for success and form validators for failures?
There was a problem hiding this comment.
Oct 2, 2019
We have toasters because we're just too lazy to write the template and the translations for error messages.
| <ds-input | ||
| id="email" | ||
| model="email" | ||
| icon="envelope" |
There was a problem hiding this comment.
Sep 30, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,59 @@
+<template>
+ <ds-form v-model="form" :schema="formSchema" @submit="submit">
+ <template slot-scope="{ errors }">
+ <ds-card :header="$t('settings.email.name')">
+ <ds-input
+ id="email"
+ model="email"
+ icon="at"envelope, please! 😉
| let response | ||
| try { | ||
| const { neode } = context | ||
| await new Validator(neode, neode.model('UnverifiedEmailAddress'), args) |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,92 @@
+import generateNonce from './helpers/generateNonce'
+import Resolver from './helpers/Resolver'
+import existingEmailAddress from './helpers/existingEmailAddress'
+import { UserInputError } from 'apollo-server'
+import Validator from 'neode/build/Services/Validator.js'
+
+export default {
+ Mutation: {
+ AddEmailAddress: async (_parent, args, context, _resolveInfo) => {
+ let response
+ try {
+ const { neode } = context
+ await new Validator(neode, neode.model('UnverifiedEmailAddress'), args)I love this!! Validators rock!!
| } = context | ||
| const { email } = args | ||
| const session = context.driver.session() | ||
| const writeTxResultPromise = session.writeTransaction(async txc => { |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,92 @@
+import generateNonce from './helpers/generateNonce'
+import Resolver from './helpers/Resolver'
+import existingEmailAddress from './helpers/existingEmailAddress'
+import { UserInputError } from 'apollo-server'
+import Validator from 'neode/build/Services/Validator.js'
+
+export default {
+ Mutation: {
+ AddEmailAddress: async (_parent, args, context, _resolveInfo) => {
+ let response
+ try {
+ const { neode } = context
+ await new Validator(neode, neode.model('UnverifiedEmailAddress'), args)
+ } catch (e) {
+ throw new UserInputError('must be a valid email')
+ }
+
+ // check email does not belong to anybody
+ await existingEmailAddress(_parent, args, context)
+
+ const nonce = generateNonce()
+ const {
+ user: { id: userId },
+ } = context
+ const { email } = args
+ const session = context.driver.session()
+ const writeTxResultPromise = session.writeTransaction(async txc => {fancy stuff here! this is the first time in the code base we are leveraging this, I believe..
There was a problem hiding this comment.
Oct 2, 2019
could you summarize the benefits of using writeTransaction and writeTxResultPromise?
or point me to some docs that explain it?
Actually, I found this // It is possible to execute write transactions that will benefit from automatic retries // on both single instance ('bolt' URI scheme) and Causal Cluster ('bolt+routing' URI scheme) from https://neo4j.com/docs/api/javascript-driver/current/
Are automatic retries the main reason you decided to use this here?
There was a problem hiding this comment.
Oct 2, 2019
Yes, I have no idea how to check those benefits. E.g. how to simulate them. But the docs clearly say: Do this. So I did.
There was a problem hiding this comment.
Oct 2, 2019
it seems like it would be a benefit to have automatic retries, and I'm in favor of following best practices. :)
| MATCH (user:User {id: $userId})-[:PRIMARY_EMAIL]->(previous:EmailAddress) | ||
| MATCH (user)<-[:BELONGS_TO]-(email:UnverifiedEmailAddress {email: $email, nonce: $nonce}) | ||
| MERGE (user)-[:PRIMARY_EMAIL]->(email) | ||
| SET email:EmailAddress |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,92 @@
+import generateNonce from './helpers/generateNonce'
+import Resolver from './helpers/Resolver'
+import existingEmailAddress from './helpers/existingEmailAddress'
+import { UserInputError } from 'apollo-server'
+import Validator from 'neode/build/Services/Validator.js'
+
+export default {
+ Mutation: {
+ AddEmailAddress: async (_parent, args, context, _resolveInfo) => {
+ let response
+ try {
+ const { neode } = context
+ await new Validator(neode, neode.model('UnverifiedEmailAddress'), args)
+ } catch (e) {
+ throw new UserInputError('must be a valid email')
+ }
+
+ // check email does not belong to anybody
+ await existingEmailAddress(_parent, args, context)
+
+ const nonce = generateNonce()
+ const {
+ user: { id: userId },
+ } = context
+ const { email } = args
+ const session = context.driver.session()
+ const writeTxResultPromise = session.writeTransaction(async txc => {
+ const result = await txc.run(
+ `
+ MATCH (user:User {id: $userId})
+ MERGE (user)<-[:BELONGS_TO]-(email:UnverifiedEmailAddress {email: $email, nonce: $nonce})
+ SET email.createdAt = toString(datetime())
+ RETURN email, user
+ `,
+ { userId, email, nonce },
+ )
+ return result.records.map(record => ({
+ name: record.get('user').properties.name,
+ ...record.get('email').properties,
+ }))
+ })
+ try {
+ const txResult = await writeTxResultPromise
+ response = txResult[0]
+ } finally {
+ session.close()
+ }
+ return response
+ },
+ VerifyEmailAddress: async (_parent, args, context, _resolveInfo) => {
+ let response
+ const {
+ user: { id: userId },
+ } = context
+ const { nonce, email } = args
+ const session = context.driver.session()
+ const writeTxResultPromise = session.writeTransaction(async txc => {
+ const result = await txc.run(
+ `
+ MATCH (user:User {id: $userId})-[:PRIMARY_EMAIL]->(previous:EmailAddress)
+ MATCH (user)<-[:BELONGS_TO]-(email:UnverifiedEmailAddress {email: $email, nonce: $nonce})
+ MERGE (user)-[:PRIMARY_EMAIL]->(email)
+ SET email:EmailAddressinteresting 🤔
| const { nonce, email } = args | ||
| const session = context.driver.session() | ||
| const writeTxResultPromise = session.writeTransaction(async txc => { | ||
| const result = await txc.run( |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,92 @@
+import generateNonce from './helpers/generateNonce'
+import Resolver from './helpers/Resolver'
+import existingEmailAddress from './helpers/existingEmailAddress'
+import { UserInputError } from 'apollo-server'
+import Validator from 'neode/build/Services/Validator.js'
+
+export default {
+ Mutation: {
+ AddEmailAddress: async (_parent, args, context, _resolveInfo) => {
+ let response
+ try {
+ const { neode } = context
+ await new Validator(neode, neode.model('UnverifiedEmailAddress'), args)
+ } catch (e) {
+ throw new UserInputError('must be a valid email')
+ }
+
+ // check email does not belong to anybody
+ await existingEmailAddress(_parent, args, context)
+
+ const nonce = generateNonce()
+ const {
+ user: { id: userId },
+ } = context
+ const { email } = args
+ const session = context.driver.session()
+ const writeTxResultPromise = session.writeTransaction(async txc => {
+ const result = await txc.run(
+ `
+ MATCH (user:User {id: $userId})
+ MERGE (user)<-[:BELONGS_TO]-(email:UnverifiedEmailAddress {email: $email, nonce: $nonce})
+ SET email.createdAt = toString(datetime())
+ RETURN email, user
+ `,
+ { userId, email, nonce },
+ )
+ return result.records.map(record => ({
+ name: record.get('user').properties.name,
+ ...record.get('email').properties,
+ }))
+ })
+ try {
+ const txResult = await writeTxResultPromise
+ response = txResult[0]
+ } finally {
+ session.close()
+ }
+ return response
+ },
+ VerifyEmailAddress: async (_parent, args, context, _resolveInfo) => {
+ let response
+ const {
+ user: { id: userId },
+ } = context
+ const { nonce, email } = args
+ const session = context.driver.session()
+ const writeTxResultPromise = session.writeTransaction(async txc => {
+ const result = await txc.run(not a fan of this naming convention... it's as if we don't know what the result of this transaction will be
we have been using transactionRes, which is maybe slightly better, but actually still makes it seem like it's a mystery what it returns...
In this case it's returning the emailNode, no?
There was a problem hiding this comment.
Oct 2, 2019
const result would be a result set. It's a local variable to store that stuff we need for the return value 😆
There was a problem hiding this comment.
Oct 2, 2019
I get that, just think we don't really gain anything by being vague, and it could make our code easier to understand if it's more expressive rather than less.
| MATCH(u:User)<-[:BELONGS_TO]-(e:UnverifiedEmailAddress {email: "new-email@example.org"}) | ||
| RETURN e | ||
| `) | ||
| const email = neode.hydrateFirst(result, 'e', neode.model('UnverifiedEmailAddress')) |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,298 @@
+import Factory from '../../seed/factories'
+import { gql } from '../../jest/helpers'
+import { getDriver, neode as getNeode } from '../../bootstrap/neo4j'
+import createServer from '../../server'
+import { createTestClient } from 'apollo-server-testing'
+
+const factory = Factory()
+const neode = getNeode()
+
+let mutate
+let authenticatedUser
+let user
+let variables
+const driver = getDriver()
+
+beforeEach(async () => {
+ variables = {}
+})
+
+beforeAll(() => {
+ const { server } = createServer({
+ context: () => {
+ return {
+ driver,
+ neode,
+ user: authenticatedUser,
+ }
+ },
+ })
+ mutate = createTestClient(server).mutate
+})
+
+afterEach(async () => {
+ await factory.cleanDatabase()
+})
+
+describe('AddEmailAddress', () => {
+ const mutation = gql`
+ mutation($email: String!) {
+ AddEmailAddress(email: $email) {
+ email
+ verifiedAt
+ createdAt
+ }
+ }
+ `
+ beforeEach(() => {
+ variables = { ...variables, email: 'new-email@example.org' }
+ })
+
+ describe('unauthenticated', () => {
+ beforeEach(() => {
+ authenticatedUser = null
+ })
+
+ it('throws AuthorizationError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { AddEmailAddress: null },
+ errors: [{ message: 'Not Authorised!' }],
+ })
+ })
+ })
+
+ describe('authenticated', () => {
+ beforeEach(async () => {
+ user = await factory.create('User', { id: '567', email: 'user@example.org' })
+ authenticatedUser = await user.toJson()
+ })
+
+ describe('email attribute is not a valid email', () => {
+ beforeEach(() => {
+ variables = { ...variables, email: 'foobar' }
+ })
+
+ it('throws UserInputError', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: { AddEmailAddress: null },
+ errors: [{ message: 'must be a valid email' }],
+ })
+ })
+ })
+
+ describe('email attribute is a valid email', () => {
+ it('creates a new unverified `EmailAddress` node', async () => {
+ await expect(mutate({ mutation, variables })).resolves.toMatchObject({
+ data: {
+ AddEmailAddress: {
+ email: 'new-email@example.org',
+ verifiedAt: null,
+ createdAt: expect.any(String),
+ },
+ },
+ errors: undefined,
+ })
+ })
+
+ it('connects `UnverifiedEmailAddress` to the authenticated user', async () => {
+ await mutate({ mutation, variables })
+ const result = await neode.cypher(`
+ MATCH(u:User)-[:PRIMARY_EMAIL]->(:EmailAddress {email: "user@example.org"})
+ MATCH(u:User)<-[:BELONGS_TO]-(e:UnverifiedEmailAddress {email: "new-email@example.org"})
+ RETURN e
+ `)
+ const email = neode.hydrateFirst(result, 'e', neode.model('UnverifiedEmailAddress'))I'll have to read up on this method... hydrateFirst sounds like some advice you give someone before going out drinking 😆
There was a problem hiding this comment.
Oct 2, 2019
😆 it's the neode way of parsing a transaction record into a neode node. It needs to know the model name and the variable in the cypher statement.
There was a problem hiding this comment.
Oct 2, 2019
great! thanks for that... I'll still have a look at the docs, and ideally use it myself... I guessed that is what the 'e' was, but I would have just used email personally... I don't know what the 4 characters less buys you, but I know programmers are different.
| <ds-space class="backendErrors" v-if="backendErrors"> | ||
| <ds-text align="center" bold color="danger">{{ backendErrors.message }}</ds-text> | ||
| </ds-space> | ||
| <ds-button icon="check" :disabled="errors" type="submit" primary> |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,113 @@
+<template>
+ <ds-card centered v-if="success">
+ <transition name="ds-transition-fade">
+ <sweetalert-icon icon="info" />
+ </transition>
+ <ds-text v-html="submitMessage" />
+ </ds-card>
+ <ds-form v-else v-model="form" :schema="formSchema" @submit="submit">
+ <template slot-scope="{ errors }">
+ <ds-card :header="$t('settings.email.name')">
+ <ds-input
+ id="email"
+ model="email"
+ icon="envelope"
+ :label="$t('settings.email.labelEmail')"
+ />
+
+ <template slot="footer">
+ <ds-space class="backendErrors" v-if="backendErrors">
+ <ds-text align="center" bold color="danger">{{ backendErrors.message }}</ds-text>
+ </ds-space>
+ <ds-button class="submit-button" icon="check" :disabled="errors" type="submit" primary>the class is now obsolete, no?
There was a problem hiding this comment.
Oct 2, 2019
Ok, you got me. I hoped to find not enough suggestions to touch this PR again, but this suggestion together with @mattwr18's suggestions above is too much to just click "Merge"...
| @@ -0,0 +1,26 @@ | |||
| import { UserInputError } from 'apollo-server' | |||
| export default async function alreadyExistingMail(_parent, args, context) { | |||
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -0,0 +1,26 @@
+import { UserInputError } from 'apollo-server'
+export default async function alreadyExistingMail(_parent, args, context) {was this name alreadyExistingMail on purpose?
Shouldn't it be alreadyExistingEmail?
or even the same as the file name? existingEmailAddress??
There was a problem hiding this comment.
Oct 2, 2019
Not on purpose. Next time I touch this file I will go with existingEmailAddress. This makes sense especially because we have multiple nodes for similar things now.
| @@ -67,9 +42,9 @@ export default { | |||
| }, | |||
| SignupByInvitation: async (_parent, args, context) => { | |||
| const { token } = args | |||
| const nonce = uuid().substring(0, 6) | |||
| const nonce = generateNonce() | |||
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -67,9 +42,9 @@ export default {
},
SignupByInvitation: async (_parent, args, context) => {
const { token } = args
- const nonce = uuid().substring(0, 6)
+ const nonce = generateNonce()nice refactor
| @@ -257,7 +257,7 @@ describe('SignupByInvitation', () => { | |||
|
|
|||
| it('throws unique violation error', async () => { | |||
| await expect(mutate({ mutation, variables })).resolves.toMatchObject({ | |||
| errors: [{ message: 'User account with this email already exists.' }], | |||
| errors: [{ message: 'A user account with this email already exists.' }], | |||
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -257,7 +257,7 @@ describe('SignupByInvitation', () => {
it('throws unique violation error', async () => {
await expect(mutate({ mutation, variables })).resolves.toMatchObject({
- errors: [{ message: 'User account with this email already exists.' }],
+ errors: [{ message: 'A user account with this email already exists.' }],nice improvement
| "reason": { | ||
| "invalid-nonce": "Is the confirmation code invalid?", | ||
| "no-email-request": "Are you certain that you requested a change of your email address?" | ||
| }, |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -159,6 +159,28 @@
"labelBio": "About You",
"success": "Your data was successfully updated!"
},
+ "email": {
+ "validation": {
+ "same-email": "This is your current email address"
+ },
+ "name": "Your email",
+ "labelEmail": "Change your email address",
+ "labelNewEmail": "New email Address",
+ "labelNonce": "Enter your code",
+ "success": "A new email address has been registered.",
+ "submitted": "An email to verify your address has been sent to <b>{email}</b>.",
+ "change-successful": "Your email address has been changed successfully.",
+ "verification-error": {
+ "message": "Your email could not be changed.",
+ "explanation": "This can have different causes:",
+ "reason": {
+ "invalid-nonce": "Is the confirmation code invalid?",
+ "no-email-request": "You haven't requested a change of your email address at all?",
+ "email-address-taken": "Has the email been assigned to another user in the meantime?"I might be confused by this... like how would my email address be assigned to someone else?
maybe it could make me think of my partner, but it might make me think I've been hacked 😮
I wonder if it wouldn't be better to just have them get in contact if they think they should be able to verify the email address, and we could explore what happened??
There was a problem hiding this comment.
Oct 2, 2019
Well, this should hardly happen. But maybe people have some kind of shared email address and they thought it's a super good idea to use that email for an account?
But you're right, it will probably be confusing. Maybe we wait for the first user feedback on this? See, if people get freaked out by this? 😉
| "explanation": "This can have different causes:", | ||
| "reason": { | ||
| "invalid-nonce": "Is the confirmation code invalid?", | ||
| "no-email-request": "Are you certain that you requested a change of your email address?" |
There was a problem hiding this comment.
Oct 2, 2019
Outdated (history rewrite) - original diff
@@ -159,6 +159,28 @@
"labelBio": "About You",
"success": "Your data was successfully updated!"
},
+ "email": {
+ "validation": {
+ "same-email": "This is your current email address"
+ },
+ "name": "Your email",
+ "labelEmail": "Change your email address",
+ "labelNewEmail": "New email Address",
+ "labelNonce": "Enter your code",
+ "success": "A new email address has been registered.",
+ "submitted": "An email to verify your address has been sent to <b>{email}</b>.",
+ "change-successful": "Your email address has been changed successfully.",
+ "verification-error": {
+ "message": "Your email could not be changed.",
+ "explanation": "This can have different causes:",
+ "reason": {
+ "invalid-nonce": "Is the confirmation code invalid?",
+ "no-email-request": "You haven't requested a change of your email address at all?",if you want to form these all as questions, I might say Are you certain you requested a change of your email address?
🍰 Pullrequest
Issues
Todo