Add prompted confirmation #30
Conversation
|
@gohkhoonhiang can you take a look and have some feedbacks? Thanks a lot 🙏 |
|
@haile-kaligo Sorry for the late response! I didn't check my Github notifications and missed this PR. Thanks for the PR! 🙏 Regarding the feature, I'm thinking that, if the dev wanted confirmation to run a job, it wouldn't hurt to have the prompt message for extra security. For that, I'm proposing we merge the 2 into a single feature, such that if a worker is configured to require a prompt, then a prompt message must be input to confirm the job run. It keeps the config simpler as there's only 1 option to configure, and the implementation more straightforward, as there's no need to check for either a WDYT? |
|
Thanks for your feed back @gohkhoonhiang, since both types serve the same purpose so it would be easier to maintain if we just only use one. So I'll remove the |
README.md
Outdated
| @@ -75,4 +75,4 @@ Note: There is no UI validation for the input, as the class inspector will not b | |||
|
|
|||
| ### Require Confirmation | |||
|
|
|||
| If the `require_confirm_worker_names` option is configured with the worker class name of the job you want to run, it will prompt for confirmation. By clicking `OK`, the job will be scheduled to run; otherwise, it will not run. This helps to prevent accidentally running a critical job when not meant to be. | |||
| If the `require_confirm_worker_names` option is configured with the worker class name of the job you want to run, it will prompt for confirmation. You will be required to type a specific keyword for the job to run, otherwise, it will not run. This keyword is set to `confirm` by default and can be customized using `require_confirm_prompt_message` option. This helps to prevent accidentally running a critical job when not meant to be. | |||
There was a problem hiding this comment.
Suggestion:
If you're using sidekiq adhoc jobs in Production, you may want to consider using this configuration as an extra protection against erroneously running a sidekiq job. Once turned on, the user will be required to enter a challenge keyword in a prompt before the job can be run through the admin UI.
To use this feature, add the worker class name of the jobs you would like to add the confirmation for into the require_confirm_worker_names option.
If you would like to configure the challenge keyword, you can use the require_confirm_prompt_message option.
There was a problem hiding this comment.
Thank you sir 🙏
There was a problem hiding this comment.
@haile-kaligo Thanks for the fixes! There's just one more small one for the README. After this, maybe you could run this branch for your app again to test the latest changes are okay, then I can release the feature. 🙏
|
|
||
| To use this feature, add the worker class name of the jobs you would like to add the confirmation for into the `require_confirm_worker_names` option. | ||
|
|
||
| The challenge keyword is set to be `confirm` as default, if you would like to configure the challenge keyword, you can use the `require_confirm_prompt_message` option. |
There was a problem hiding this comment.
@haile-kaligo One last fix for the README. Could you add the option description to the Options list above under Usage section? Then all should be good.
haile-kaligo
force-pushed
the
feature/add_prompted_confirmation
branch
from
cf32890
to
c337197
Compare
|
Self-QA
|
Background
Need to require more attention when confirming to run a job, for an extra assurance that very critical jobs won't be triggered by mistake.
What's this change
require_confirm_prompt_message: The keyword for the confirmation (preferably an environment variable)Running confirmation required job
Requires user to type a challenge keyword
