Skip to content

Conversation

@miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Nov 6, 2024

All Submissions:

Changes proposed in this Pull Request:

1205919985867982-as-1208628153846581/f

Implements changes from #3492 to the IA project.

How to test the changes in this Pull Request:

  1. Navigate to "Newspack -> Settings" and at the Connections tab, scroll to the Webhooks section
  2. Click to create a new endpoint and confirm the "Global" option is no longer available
  3. Attempt to save without a URL or selecting any actions
  4. Confirm the modal scrolls to the top and you see an error message
  5. Fill a random string (invalid URL) and attempt to save
  6. Confirm you get an error due to invalid format
  7. Put a valid URL and select some actions
  8. Confirm you can save the endpoint
  9. Refresh the page and confirm the endpoint persist

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Comment on lines -36 to 38
return `${ __(
'Endpoint disabled due to error:',
'Endpoint disabled due to error',
'newspack-plugin'
) }: ${ endpoint.disabled_error }`;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was printing two :, removed the one from the localized string

@miguelpeixe miguelpeixe marked this pull request as ready for review November 6, 2024 20:05
@miguelpeixe miguelpeixe requested a review from a team as a code owner November 6, 2024 20:05
@miguelpeixe miguelpeixe self-assigned this Nov 6, 2024
Comment on lines -81 to +90
url: string | undefined,
url: string,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this would ever be undefined so I removed it

Copy link
Contributor

@jaredrethman jaredrethman left a comment

Choose a reason for hiding this comment

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

@miguelpeixe code looks good, I made one non-blocking comment about validating URLs below. Feel free to keep or change as you think!

* @param url The URL to validate.
* @return Error message if URL is invalid, false otherwise.
*/
export function validateUrl( url: string ): string | false {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be modernized to use URL interface for example:

export function validateUrl( url: string ): string | false {
	if ( ! url ) {
		return __( 'URL is required.', 'newspack-plugin' );
	}
	try {
		const urlObject = new URL( url );
		// Ensure the protocol is either http or https
		if ( ! [ 'http:', 'https:' ].includes( urlObject.protocol ) ) {
			return __( 'URL must use HTTP or HTTPS protocol.', 'newspack-plugin' );
		}
		return false;
	} catch ( error ) {
		return __( 'Invalid URL format.', 'newspack-plugin' );
	}
}

There is also a function inside @wordpress/url called isUrl which also validates urls using the above pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! Updated in 9eb2589. While at it, I also restricted to HTTPS, which is enforced in the backend.

@github-actions github-actions bot added the [Status] Approved The pull request has been reviewed and is ready to merge label Nov 7, 2024
Copy link
Contributor

@jaredrethman jaredrethman left a comment

Choose a reason for hiding this comment

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

Code+changes look great, approved!

@miguelpeixe miguelpeixe merged commit e2ccdee into epic/ia Nov 19, 2024
@miguelpeixe miguelpeixe deleted the feat/ia-webhooks-deprecate-global-endpoint branch November 19, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants