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

fix: bug where preview button wouldn't show up for users #576

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

veryspry
Copy link
Contributor

@veryspry veryspry commented Nov 2, 2021

Due to some confusing naming that is in place to ensure backwards compatibility for Gatsby users who have not yet moved to Content Sync preview, we accidentally introduced a regression through #569. This happened because we had references to a configuration value called previewWebookUrl which was essentially unused. The correct name for the preview webhook url in the config object is webhookUrl (this is the confusing backwards compatibility naming.)

This bug is causing a missing url warning to display even if the "Preview Url" field in the config is filled out since that field is mapped to the webhookUrl and not the previewWebhookUrl config value.

contentful

This PR fixes that bug and also removes references to the superfluous config item previewWebhookUrl. @Jwhiles sorry for any confusion here and with your recent fix! I should have caught that in your PR but had already forgotten about the historical context around webhookUrl (naming is important 😅) and I'm hoping that these changes help us to avoid a similar mistake in the future.

@veryspry veryspry requested a review from a team as a code owner November 2, 2021 17:51
authToken,
} = this.state;

const validPreview = !previewUrl || isValidUrl(previewUrl);
const validContentSync = !contentSyncUrl || isValidUrl(contentSyncUrl);
const validWebhook = !webhookUrl || isValidUrl(webhookUrl);
const validPreviewWebhook = !previewWebhookUrl || isValidUrl(previewWebhookUrl);
Copy link
Contributor Author

@veryspry veryspry Nov 2, 2021

Choose a reason for hiding this comment

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

As it turns out my fix in #570 was not needed and actually never added in the first place since we didn't use the value anywhere. What we really needed was to remove previewWebhookUrl altogether.

Copy link

@zcei zcei left a comment

Choose a reason for hiding this comment

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

Unblocking Gatsby during Berlin night hours, the change only affect their app's subfolder.

@zcei zcei merged commit 9b84aed into contentful:master Nov 2, 2021
@TylerBarnes
Copy link
Contributor

Thank you @zcei! We appreciate you helping us out 😄

@veryspry
Copy link
Contributor Author

veryspry commented Nov 3, 2021

Thanks for merging this after hours 😀

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.

4 participants