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.

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

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

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 requested review from ericholscher, humitos and Aug 14, 2019
@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

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.

Loading

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

@ericholscher ericholscher Aug 19, 2019

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.

Loading

Loading
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Aug 19, 2019

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

Loading

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Aug 20, 2019

@ericholscher ready for a full review :)

Loading

@saadmk11 saadmk11 requested a review from Aug 20, 2019
Copy link
Member

@ericholscher ericholscher left a comment

This looks good with one small change 👍

Loading

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

@ericholscher ericholscher Aug 20, 2019

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.

Loading

Copy link
Member Author

@saadmk11 saadmk11 Aug 20, 2019

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. :)

Loading

Copy link
Member Author

@saadmk11 saadmk11 Aug 20, 2019

Choose a reason for hiding this comment

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

@ericholscher Updated the PR!

Loading

@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
2 checks passed
Loading
@saadmk11 saadmk11 deleted the webhook-sync branch Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants