-
Notifications
You must be signed in to change notification settings - Fork 191
Add overwrite prompt for store export if file exists #6114
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
Conversation
Coverage report
Test suite run success3031 tests passing in 1314 suites. Report generated by 🧪jest coverage report action from 3d61b1b |
0044cb0
to
0dc8cdd
Compare
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
outputInfo('Exiting.') | ||
process.exit(0) | ||
} | ||
} else if (!(await confirmExportPrompt(sourceShopDomain, toFile))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am slightly concerned that with file conflict we are getting less informative confirmation that is only asking to override the file and export prompt is not being shown.
I would make it so export confirmation prompt is displayed first and then when user confirmed the intent to export from a store to a file we show the next prompt to confirm file overwrite.
nit, I would extract the confirmation logic into separate function since it becomes really bulky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other issue I can see with it is that no-prompt
mode will automatically overwrite the file. I would expect it to actually fail with the error stating that the file already exists. I think no-prompt
should be still safe. E.g. when I use it in automation I would love it to not silently overwrite the file if I messed up with the logic. I don't have strong opinions on that so I would love to hear what other people think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I paired on this with @Rusty-UX, so will let him chime in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 @rzaharenkov We discussed having the overwrite then the export prompt. IMO the proposed option in the PR is the strongest, because it clarifies the export operation will proceed, while also communicating the overwrite. IMO it's less verbose, but the information communicated is essentially the same.
On the comment of no-prompt
I have less of an opinion on this, but I agree with you that it should fail instead of silently overwriting. LMK if that means we need to rethink the prompt content displayed for the export+overwrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I am good to have a single confirmation, I think it's more user friendly.
nit, I would love to see some kind of extracting this logic. Maybe it could be arguments passed to the same function confirmExportPrompt
, or it could be another function. But not a blocker for this PR.
Also would love a change to avoid silent overwrite for no-prompt
mode. But it could definitely be a follow up since this behavior is not changing in this PR and it's positive incremental change.
outputInfo('Exiting.') | ||
process.exit(0) | ||
} | ||
} else if (!(await confirmExportPrompt(sourceShopDomain, toFile))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I am good to have a single confirmation, I think it's more user friendly.
nit, I would love to see some kind of extracting this logic. Maybe it could be arguments passed to the same function confirmExportPrompt
, or it could be another function. But not a blocker for this PR.
Also would love a change to avoid silent overwrite for no-prompt
mode. But it could definitely be a follow up since this behavior is not changing in this PR and it's positive incremental change.
@@ -36,7 +38,12 @@ export class StoreExportOperation implements StoreOperation { | |||
const apiShopId = await this.validateShop(sourceShopDomain) | |||
|
|||
if (!flags['no-prompt']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this section is a little unwieldy; I would prefer to have this operation just call confirmExportPrompt(sourceShopDomain, toFile)
and then have the prompt sort out whether it needs confirm about the overwrite or not.
the unit test for the operation can just test that it calls the confirmExport prompt with the right parameters;
and the confirmExportPrompt tests can confirm that asks the right questions based on no-prompt flags, file existence etc
0dc8cdd
to
e246a3c
Compare
547dbbb
to
b438c68
Compare
e246a3c
to
5095851
Compare
904f6c8
to
e81e08a
Compare
5095851
to
3d61b1b
Compare
WHY are these changes introduced?
Fixes https://github.com/Shopify/workflows-operations/issues/3364
WHAT is this pull request doing?
Adds a confirmation prompt when attempting to export store data to a file that already exists. This prevents accidental overwriting of existing files by:
confirmExportPrompt
function so that it asks users if they want to overwrite an existing file if the file existsHow to test your changes?
run this command twice, the first time you will see the normal flow, the second time you will see the overwrite flow
pnpm shopify store copy --from-store=source --to-file=file --mock
Measuring impact
How do we know this change was effective? Please choose one:
Checklist