Skip to content
This repository has been archived by the owner on Jul 17, 2023. It is now read-only.

#33 migrate to inquirer #46

Merged
merged 5 commits into from
Jun 1, 2020
Merged

#33 migrate to inquirer #46

merged 5 commits into from
Jun 1, 2020

Conversation

lvenier
Copy link
Contributor

@lvenier lvenier commented Jun 1, 2020

No description provided.

@lvenier
Copy link
Contributor Author

lvenier commented Jun 1, 2020

@martinlevesque : might require few adjustement as inquirer handle input validation in a different way.
validate() function might be added...

modules/auth.js Outdated
return apiRequest.post('account/register', {
account: {
email,
password,
password_confirmation,
passwordConfirmation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be changed to:

password_confirmation: passwordConfirmation,

the variable name is important

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I got it right. you mean the variable name ?
I might have missed something as the var name could be anything ?

Copy link
Member

@martinlevesque martinlevesque Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently calling the api to register an account with:

{
account: {
 ...
passwordConfirmation,
...
}
}

but the API looks for:

{
account: {
 ...
password_confirmation,
...
}
}

and so to fix it:

{
account: {
 ...
password_confirmation: passwordConfirmation,
...
}
}

modules/auth.js Outdated
choices: ['y', 'n'],
default: 'y'
}]
const result = await inquirer.prompt(schema)
return new Promise((resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace this return by (and remove the new Promise(...)):

return result.wantsNewsletter === 'y' ? 1 : 0

@martinlevesque
Copy link
Member

thanks for your contribution, the UI is good.
Made 2 comments, 2 bugs to fix.

@lvenier
Copy link
Contributor Author

lvenier commented Jun 1, 2020

@martinlevesque : ok got it. PR updated.

@martinlevesque martinlevesque merged commit 01e81d4 into openode-io:master Jun 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants