Skip to content

Auto Sync and Re-Sync for Manually Created Integrations #6071

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

Merged
merged 12 commits into from
Aug 20, 2019

Conversation

saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Aug 13, 2019

A lot of work needs to be done and a lot of cases needs to be handled before its ready for review. :)
closes #3932

related to the first step of #6052

@saadmk11 saadmk11 added the PR: work in progress Pull request is not ready for full review label Aug 13, 2019
@saadmk11 saadmk11 requested review from ericholscher, humitos and a team August 14, 2019 15:14
@saadmk11 saadmk11 changed the title Auto Sync and Re-Sync Manually Created integrations Auto Sync and Re-Sync for Manually Created Integrations Aug 15, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks good, I think we can do a bit more to make this flow nice, by showing the user the webhook URL & secret always.

integration is not functioning correctly, try resyncing the webhook:
This webhook was configured when this project was imported
or it was manually created with correct configuration. If this
integration is not functioning correctly, try re-syncing the webhook:
Copy link
Member

Choose a reason for hiding this comment

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

This looks good, but I'd like to see us always displaying the webhook URL information, so that users can confirm that the automated webhook is the one they see in their GitHub admin. We should always show this data.

@ericholscher
Copy link
Member

BTW, this is so much better than making users login to GitHub and create the webhook manually. This is a huge win!

@saadmk11 saadmk11 removed the PR: work in progress Pull request is not ready for full review label Aug 20, 2019
@saadmk11 saadmk11 requested a review from ericholscher August 20, 2019 09:05
@saadmk11
Copy link
Member Author

@ericholscher ready for a full review :)

@saadmk11 saadmk11 requested a review from a team August 20, 2019 09:05
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks good with one small change 👍

user_pk=self.request.user.pk,
integration=self.object
)
if self.object.integration_type != Integration.API_WEBHOOK:
Copy link
Member

Choose a reason for hiding this comment

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

Do we not have some kind of has_sync check here? I feel like this might break in the future as we add new types of Integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I think what you said would be better. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ericholscher Updated the PR!

@saadmk11 saadmk11 mentioned this pull request Aug 20, 2019
3 tasks
@saadmk11 saadmk11 closed this Aug 20, 2019
@saadmk11 saadmk11 reopened this Aug 20, 2019
@ericholscher ericholscher merged commit 5c03027 into readthedocs:master Aug 20, 2019
@saadmk11 saadmk11 deleted the webhook-sync branch August 20, 2019 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way back after deleting the webhook integration
2 participants