Skip to content

Conversation

@ivan-ottinger
Copy link
Contributor

@ivan-ottinger ivan-ottinger commented Nov 21, 2024

Related issues

Proposed Changes

  • automatically start site after import

Testing Instructions

  1. Check out the PR and build the app with npm start.
  2. Navigate to the Import / Export tab and create an export.
  3. Make some visible changes to the site's homepage (e.g. change the site title or some colors).
  4. Stop the site.
  5. Navigate to the Import / Export tab again and upload a previously-exported site to initiate the import.
  6. Once the import finishes, the site should start automatically.
  7. If you go to the Overview tab, the site thumbnail should be updated to match the originally-exported site (without the customizations).

Pre-merge Checklist

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

@ivan-ottinger ivan-ottinger self-assigned this Nov 21, 2024
@ivan-ottinger ivan-ottinger requested a review from a team November 21, 2024 11:17
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the change.
I confirm that after importing a backup to a stopped site results in starting that site.

I left a suggestion to add a new check for this new behaviour.

m73fKd.mp4

} );
( useSiteDetails as jest.Mock ).mockReturnValue( {
updateSite: jest.fn(),
startServer: jest.fn(),
Copy link
Member

Choose a reason for hiding this comment

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

What do you think if we add a new test or at least a new assert to check we called the startServer after an import?

Something like: expect( useSiteDetails().startServer ).toHaveBeenCalledTimes( 1 );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for testing and reviewing the PR, Antonio!

What do you think if we add a new test or at least a new assert to check we called the startServer after an import?
Something like: expect( useSiteDetails().startServer ).toHaveBeenCalledTimes( 1 );

I like the idea! I have made a change in cd93a5f and also added testing whether startServer was called with the correct site ID.

@ivan-ottinger ivan-ottinger merged commit 8957b3f into trunk Nov 22, 2024
15 checks passed
@ivan-ottinger ivan-ottinger deleted the update/start-site-after-import branch November 22, 2024 14:19
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.

3 participants