Skip to content

Conversation

@sejas
Copy link
Member

@sejas sejas commented Apr 25, 2024

Proposed Changes

  • It sends the local WordPress version when a Demo site is created or updated to match the same wp version

Testing Instructions

  • Apply and sandbox the API D146651-code
  • Select any site
  • Ideally change your local WordPress version to a different one. You can use wp core update --version=6.4.1 --force
  • Click on Share tab
  • Create a new demo site
  • Wait until the process finishes
  • Enter on /wp-admin on wp.build site
  • Observe the WordPress version matches your local site
demo.sites.match.local.wordpress.site.mp4

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@sejas sejas changed the title Demo Sites: send WordPress version on archiving Demo Sites: Ensure demo site uses WP version same as local site Apr 25, 2024
@sejas sejas self-assigned this Apr 25, 2024
Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

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

The code change looks clean. I left two minor comments.

Could we send the WP version with the demo site update request, too?

const formData = [ [ 'import', file ] ];
const wordpressVersion = await getIpcApi().getWpVersion( siteId );
if ( wordpressVersion.length >= 3 ) {
// Minimum version length is '6.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: the condition is clear enough, so we don't need the comment

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 for confirming it. I was not sure if it was clear enough.
I was thinking to make a comparison like, but I preferred to add it as a comment.
if ( wordpressVersion.length >= 'x.y'.length ) {

Anyway, if it's not clear for everybody, now they can read these comments 👍

Removed the comment: d58a3ac

}

export async function getWpVersion( _event: IpcMainInvokeEvent, wordPressPath: string ) {
export async function getWpVersion( _event: IpcMainInvokeEvent, id: string ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update

getWpVersion: ( wordPressPath: string ) => ipcRenderer.invoke( 'getWpVersion', wordPressPath ),
as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!, thanks for the early feedback.

Changed 691fa9a

@sejas sejas requested a review from a team April 26, 2024 19:34
@katinthehatsite
Copy link
Contributor

I am testing the issue that this is what I am getting on my end (in combination with the backend diff):

Screenshot 2024-04-29 at 11 08 28 AM

I am not sure if it is because the code needs to be in production to confirm the WordPress version of the demo site or if I am testing something incorrectly on my end. If the code needs to be in production, what is the best way to confirm that the version is set up correctly on the demo site? Thanks!

@sejas
Copy link
Member Author

sejas commented Apr 29, 2024

@katinthehatsite, thanks for reviewing it!. The best way is running a query on our end. I updated the description on D146651-code, that seems it was overwritten with an old version.

Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

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

I tested this with the server-side diff, and everything worked as expected.

@sejas sejas merged commit 91f982b into trunk Apr 29, 2024
@sejas sejas deleted the update/send-wp-version-on-archive branch April 29, 2024 15:36
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