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

[Data transfer] aborting prompts displays as error #15974

Merged
merged 3 commits into from Mar 8, 2023

Conversation

innerdvations
Copy link
Contributor

What does it do?

When a user doesn't enter data for a prompt, the error message works as described below.

Why is it needed?

As a shipper

  • when I run the transfer
    • when I don’t put any transfer token
      • then I want the message No token entered, aborting transfer to be in red, and no information about the process's completion time
    • when I select “N” (no) to the question The transfer will delete all data in the remote database and media files. Are you sure you want to proceed?
      • then I want the error message in red Transfer process aborted

How to test it?

see above

@innerdvations innerdvations marked this pull request as ready for review March 2, 2023 16:44
@innerdvations innerdvations self-assigned this Mar 2, 2023
@innerdvations innerdvations added source: core:data-transfer Source is core/data-transfer package pr: enhancement This PR adds or updates some part of the codebase or features labels Mar 2, 2023
@innerdvations innerdvations added this to the 4.7.2 milestone Mar 2, 2023
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage has no change and project coverage change: +9.07 🎉

Comparison is base (6b493cd) 51.61% compared to head (606a88e) 60.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15974      +/-   ##
==========================================
+ Coverage   51.61%   60.68%   +9.07%     
==========================================
  Files         374     1495    +1121     
  Lines       14091    36876   +22785     
  Branches     3168     7355    +4187     
==========================================
+ Hits         7273    22380   +15107     
- Misses       5617    12415    +6798     
- Partials     1201     2081     +880     
Flag Coverage Δ
back 51.35% <ø> (ø)
front 66.30% <ø> (?)
unit_back 51.35% <ø> (ø)
unit_front 66.30% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/core/admin/admin/src/pages/ProfilePage/index.js 63.71% <0.00%> (ø)
...onents/RepeatableComponent/components/Component.js 91.66% <0.00%> (ø)
...missions/admin/src/pages/Providers/tests/server.js 100.00% <0.00%> (ø)
...etDialog/BrowseStep/PaginationFooter/Pagination.js 100.00% <0.00%> (ø)
...ges/SettingsPage/pages/Users/ListPage/utils/api.js 77.77% <0.00%> (ø)
...dmin/src/components/DataManagerProvider/reducer.js 97.19% <0.00%> (ø)
...es/AuthPage/components/FieldActionWrapper/index.js 100.00% <0.00%> (ø)
...ngsPage/components/Tokens/TokenTypeSelect/index.js 87.50% <0.00%> (ø)
...ingsPage/pages/Webhooks/ProtectedEditView/index.js 0.00% <0.00%> (ø)
...es/core/upload/admin/src/utils/displayedFilters.js 100.00% <0.00%> (ø)
... and 1111 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -116,7 +117,7 @@ const confirmMessage = (message) => {
},
]);
if (!answers.confirm) {
exitWith(0);
exitWith(1, failMessage);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a default value for the fail message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended as a generic way to confirm a prompt, I'm not sure we could come up with text that wouldn't be more annoying to have to override every time.

I'm already not completely convinced on this being an error exit code, because if the user is specifically saying "no" it's not an error, it's working as intended. (and I at least want to keep the link between "red message" and "error" and not deviate from that)

Copy link
Member

@Convly Convly Mar 7, 2023

Choose a reason for hiding this comment

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

I'm already not completely convinced on this being an error exit code
Yeah, this is a tricky one.

Here are some examples about exit codes for canceled commands
image

Copy link
Member

Choose a reason for hiding this comment

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

Not sure which one we should use though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm interpreting this right:
First example you cancelled during a prompt and got a 0.
Second example you cancelled while it was working and got a 1.
Third example was cancelled during a password prompt for sudo and got a 1.

I think my preference is for a 0/warning, but I'm not opposed to a 1/error, so I think we should just go with it since it makes it easier for anything reading the exit code to interpret it as "ok, there was an error so the transfer didn't succeed" and also lets the user know the same with red text.

@innerdvations innerdvations merged commit 49df2f4 into main Mar 8, 2023
@Convly Convly deleted the enhancement/transfer-error-messaging branch March 10, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: enhancement This PR adds or updates some part of the codebase or features source: core:data-transfer Source is core/data-transfer package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants