Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Documentation for build notifications using webhooks. #3671

Merged
merged 6 commits into from
Apr 17, 2018
Merged

Documentation for build notifications using webhooks. #3671

merged 6 commits into from
Apr 17, 2018

Conversation

aasis21
Copy link
Contributor

@aasis21 aasis21 commented Feb 23, 2018

Regarding #3670

stsewd
stsewd reviewed Feb 23, 2018
View reviewed changes
docs/guides/build-notifications.rst Outdated
@@ -1,13 +1,46 @@
Enabling Build Notifications
============================

Using Email
******************
Copy link
Member

Choose a reason for hiding this comment

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

Use ------ for subheadings with the same same size of the text, and should be sentence case.

You can read more about this here https://docs.readthedocs.io/en/latest/docs.html.

stsewd
stsewd reviewed Feb 23, 2018
View reviewed changes
docs/guides/build-notifications.rst Outdated
.. code-block:: json

{
'name': project.name,
Copy link
Member

Choose a reason for hiding this comment

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

Json use " (double quotes).

What about putting real values here? Something like

{
    "name": "Read the Docs",
    "slug": "rtd",
    ...
}

@aasis21
Copy link
Contributor Author

aasis21 commented Feb 24, 2018

Thanks, @stsewd for the quick review.
I have made the changes as you suggested.

humitos
humitos reviewed Feb 24, 2018
View reviewed changes
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

Left 3 small comments. After them, we could merge.

docs/guides/build-notifications.rst Outdated
* Fill in the **Url** field under the **New Webhook Notifications** heading
* Submit the form

The project name, id and its bulid instance that failed will be sent to your webhook url:
Copy link
Member

Choose a reason for hiding this comment

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

We should mention here that this is a POST request, maybe.

Copy link
Member

Choose a reason for hiding this comment

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

Also, it doesn't send the project id, but the project slug.

docs/guides/build-notifications.rst
"date": "2017-02-15 20:35:54",
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Remove these extra spaces, please (just leave one).

stsewd
stsewd reviewed Feb 24, 2018
View reviewed changes
docs/guides/build-notifications.rst Outdated
@@ -1,13 +1,46 @@
Enabling Build Notifications
============================

Using Email
Copy link
Member

Choose a reason for hiding this comment

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

Sentence case here: Using email also Using webhook

stsewd
stsewd reviewed Feb 24, 2018
View reviewed changes
docs/guides/build-notifications.rst Outdated
* Fill in the **Url** field under the **New Webhook Notifications** heading
* Submit the form

The project name, slug and its bulid instance that failed will be sent as POST request to your webhook url:
Copy link
Member

Choose a reason for hiding this comment

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

Little typo here its bulid -> its build

@aasis21
Copy link
Contributor Author

aasis21 commented Feb 26, 2018

@stsewd, I have made the suggested changes. Hoping that this can be merged now : )

agjohnson
agjohnson requested changes Feb 27, 2018
View reviewed changes
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

These updates look great! I added some copyedit changes to grammar etc.

docs/guides/build-notifications.rst Outdated

* Going to **Admin > Notifications** in your project.
* Fill in the **Email** field under the **New Email Notifications** heading
* Submit the form

You should now get notified when your builds fail!
You should now get notified on your email when your builds fail!
Copy link
Contributor

Choose a reason for hiding this comment

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

'on your' -> 'by'

docs/guides/build-notifications.rst Outdated
Using webhook
-------------

Read the Docs also allows webhooks configuration to receive notification regarding builds fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified to 'Read the Docs can also send webhooks when builds fail.'

docs/guides/build-notifications.rst Outdated

Read the Docs also allows webhooks configuration to receive notification regarding builds fails.

Take these steps to enable build notifications using webhook:
Copy link
Contributor

Choose a reason for hiding this comment

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

'using webhook' -> 'using a webhook'

docs/guides/build-notifications.rst Outdated

Take these steps to enable build notifications using webhook:

* Going to **Admin > Notifications** in your project.
Copy link
Contributor

Choose a reason for hiding this comment

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

'Going to' -> 'Go to'

docs/guides/build-notifications.rst Outdated
Take these steps to enable build notifications using webhook:

* Going to **Admin > Notifications** in your project.
* Fill in the **Url** field under the **New Webhook Notifications** heading
Copy link
Contributor

Choose a reason for hiding this comment

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

'Url' -> 'URL'

docs/guides/build-notifications.rst Outdated
* Fill in the **Url** field under the **New Webhook Notifications** heading
* Submit the form

The project name, slug and its build instance that failed will be sent as POST request to your webhook url:
Copy link
Contributor

Choose a reason for hiding this comment

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

'its build instance' could be slightly clearer as 'details for the build'

also: 'url' -> 'URL'

@aasis21
Copy link
Contributor Author

aasis21 commented Feb 27, 2018

Thanks, @agjohnson for the review. I have made changes as you suggested.

@aasis21
Copy link
Contributor Author

aasis21 commented Mar 6, 2018

@agjohnson, @humitos, @stsewd, Any new changes or suggestion regarding this.

humitos
humitos reviewed Apr 17, 2018
View reviewed changes
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks!

@humitos
Copy link
Member

humitos commented Apr 17, 2018

All the requested changed from @agjohnson were already done. So, I'm merging this PR. Thanks for your contribution @aasis21!

@humitos humitos merged commit 8006182 into readthedocs:master Apr 17, 2018
@stsewd stsewd mentioned this pull request May 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humitos humitos humitos left review comments

@stsewd stsewd stsewd left review comments

@agjohnson agjohnson agjohnson requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aasis21 @humitos @agjohnson @stsewd @RichardLitt