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

Documentation for build notifications using webhooks. #3671

Merged
merged 6 commits into from Apr 17, 2018

Conversation

Projects
None yet
5 participants
@aasis21
Contributor

aasis21 commented Feb 23, 2018

Regarding #3670

aasis21 added some commits Feb 23, 2018

Merge pull request #1 from aasis21/Documentation-for-build-notificati…
…ons-using-webhooks

Documentation for build notifications using webhooks
@@ -1,13 +1,46 @@
Enabling Build Notifications
============================
Using Email
******************

This comment has been minimized.

@stsewd

stsewd Feb 23, 2018

Member

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.

.. code-block:: json
{
'name': project.name,

This comment has been minimized.

@stsewd

stsewd Feb 23, 2018

Member

Json use " (double quotes).

What about putting real values here? Something like

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

This comment has been minimized.

Contributor

aasis21 commented Feb 24, 2018

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

@humitos

Thanks for this PR!

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

* 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:

This comment has been minimized.

@humitos

humitos Feb 24, 2018

Member

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

This comment has been minimized.

@humitos

humitos Feb 24, 2018

Member

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

"date": "2017-02-15 20:35:54",
}
}

This comment has been minimized.

@humitos

humitos Feb 24, 2018

Member

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

@@ -1,13 +1,46 @@
Enabling Build Notifications
============================
Using Email

This comment has been minimized.

@stsewd

stsewd Feb 24, 2018

Member

Sentence case here: Using email also Using webhook

* 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:

This comment has been minimized.

@stsewd

stsewd Feb 24, 2018

Member

Little typo here its bulid -> its build

@aasis21

This comment has been minimized.

Contributor

aasis21 commented Feb 26, 2018

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

@agjohnson

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

* 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!

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

'on your' -> 'by'

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

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

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

Read the Docs also allows webhooks configuration to receive notification regarding builds fails.
Take these steps to enable build notifications using webhook:

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

'using webhook' -> 'using a webhook'

Take these steps to enable build notifications using webhook:
* Going to **Admin > Notifications** in your project.

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

'Going to' -> 'Go to'

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

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

'Url' -> 'URL'

* 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:

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

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

also: 'url' -> 'URL'

@aasis21

This comment has been minimized.

Contributor

aasis21 commented Feb 27, 2018

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

@aasis21

This comment has been minimized.

Contributor

aasis21 commented Mar 6, 2018

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

@humitos

This comment has been minimized.

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 rtfd:master Apr 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment