-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Notify integration guidance #574
Conversation
docs/documentation/using-notify.md
Outdated
@@ -0,0 +1,48 @@ | |||
To set it locally, run this command in your Terminal, in the root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like there's some context missing here – I don't know what Notify is, and whether I want to (or need to) use it. The guidance is talking about 'setting it' but I have no idea what 'it' is.
docs/documentation/using-notify.md
Outdated
```shell | ||
echo NOTIFYAPIKEY=xxxxxxx >> .env | ||
```` | ||
(where xxxxxxx is a key you’ve created in Notify). This creates a file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases, .env should already exist thanks to
Lines 46 to 52 in 9953d9d
// Create template .env file if it doesn't exist | |
const envExists = fs.existsSync(path.join(__dirname, '/.env')) | |
if (!envExists) { | |
fs.createReadStream(path.join(__dirname, '/lib/template.env')) | |
.pipe(fs.createWriteStream(path.join(__dirname, '/.env'))) | |
} | |
} |
If I had already customised that file and didn't know what that command did, I'd be nervous that it'd wipe out my existing env configuration.
docs/documentation/using-notify.md
Outdated
(where xxxxxxx is a key you’ve created in Notify). This creates a file | ||
named `.env` which contains your API key. | ||
|
||
To set it on Heroku, go to the settings page on your app, click |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it clear that users will need to do this for Heroku (if they're using Heroku) as well as locally, because their .env file won't (shouldn't) be committed? At the minute it could be interpreted as an either/or thing.
docs/documentation/using-notify.md
Outdated
This code goes at the bottom of routes.js: | ||
|
||
```javascript | ||
router.post('/page-with-email-or-phone-input', function (req, res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call out that this needs to be the same URL that the form POSTs to?
Thanks for the comments @36degrees. I will tidy this up when I have a bit more time… |
I’ve rewritten the instructions to add a lot more context. The previous instructions were a very bare-bones draft which assumed a lot of knowledge. Hopefully this is enough so that someone who is confident using the prototype kit can integrate their prototype with Notify. |
docs/documentation/using-notify.md
Outdated
|
||
You can use GOV.UK Notify to send text messages or emails when users | ||
interact with your prototype. For example you could send users a | ||
confirmation email sent at the end of a journey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repetition of 'sent'/'send'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/documentation/using-notify.md
Outdated
|
||
To save the key on your computer, run this command in your Terminal, in | ||
the root folder of your prototype (where xxxxxxx is a key you’ve copied | ||
from Notify): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be less daunting to just tell people to add this line to end of .env
- and they'll use whatever tool is familiar to them. The only downside is there's a possibility .env
doesn't exist (before running for the first time), though if so people just need to run the kit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/documentation/using-notify.md
Outdated
### Keeping your key safe | ||
|
||
It’s really important that you keep your key secret. If you put it in | ||
the `.env` file it’s safe – that file isn’t published on Github. If you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github > GitHub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/documentation/using-notify.md
Outdated
notify = new NotifyClient(process.env.NOTIFYAPIKEY); | ||
``` | ||
|
||
`process.env.NOTIFYAPIKEY` is a special kind of variable that Node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure users need to know this? Might be simpler to leave it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/documentation/using-notify.md
Outdated
Make a page with a form to collect the user’s email address. For | ||
example: | ||
``` | ||
{% from "input/macro.njk" import govukInput %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we replace this with the HTML for now? We're not really pushing the macros as the main approach yet, as the error handling is really bad and we don't cover them in the training yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/documentation/using-notify.md
Outdated
|
||
Save this page as `email-address-page.html`. | ||
|
||
Then add this code at the bottom of routes.js: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be above module.exports = router
I wonder if there's a way to provide all this code as examples, though I know we can't distribute a Notify key. Hmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks Chris! It would be great for one of us to do some guerilla testing and see if someone can follow these instructions easily and send an email. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this and it works well – though the example form doesn't extend the layout so if you blindly copy the instructions you get an unstyled form without the header, footer, etc.
The previous instructions were a very bare-bones draft which assumed a lot of context. This commit attempts to rewrite them for someone without knowledge of what Notify is, or how an API works. Hopefully this is enough so that someone who is confident using the prototype kit can integrate their prototype with Notify.
Adapted from: https://gist.github.com/quis/1136d255a5cdc47f4b72bc5bf234cf97