Prompt for operator name #721
Conversation
artoonie
force-pushed
the
feature/issue-631_prompt-for-name
branch
from
54748d9
to
38a7771
Compare
| if (result.isPresent()) { | ||
| Logger.info("Name(s) of operators, as entered interactively: " + result.get()); | ||
| } else { | ||
| Logger.info("Operator(s) did not enter a name."); |
There was a problem hiding this comment.
Should we block this / throw an exception here instead?
There was a problem hiding this comment.
I prefer this, since when developing, I often hit tabulate a lot and can just hit cancel.
My reasoning is that we can't guarantee they'll enter a correct name, so why pretend to validate? They could enter foo instead of empty string. Just let them do empty string.
There was a problem hiding this comment.
Ok, but if that's the case, why do we force the user to provide a value in the CLI version? Shouldn't we treat this consistently?
I'm ok with allowing the user to bypass without entering a name but, if they do, we should probably log a warning (or maybe just an info) saying the user didn't provide a name.
There was a problem hiding this comment.
We already log an INFO that user didn't provide a name (Operator(s) did not enter a name.)
There was a problem hiding this comment.
I can update the CLI to not fail on empty string, but I fear it's more hidden there. Especially when running via gradle, the line <===========--> 85% EXECUTING [20s] shows up after the request for a name, so it could be hidden.
In other words, empty string in CLI is more likely to be accidental, whereas in tabulation, it's more obvious.
Happy to make changes in either direction!
There was a problem hiding this comment.
I think an argument in favor of throwing an exception / requiring this in both cases is that the user might accidentally bypass the message box with a mis-click or mis-key. I say we should go forward with it. It's really easy to just enter "asdf" when testing here anyway.
There was a problem hiding this comment.
Or aoeu, depending on keyboard layout ;)
There was a problem hiding this comment.
Whoa whoa whoa, are you one of those? I'm impressed... 👽
|
All comments addressed -- entering no name or canceling out of the name prompt cancels tabulation now |
artoonie
force-pushed
the
feature/issue-631_prompt-for-name
branch
from
9f36ec8
to
b475089
Compare
| if (result.isPresent()) { | ||
| Logger.info("Name(s) of operators, as entered interactively: " + result.get()); | ||
| } else { | ||
| Logger.info("Operator(s) did not enter a name."); |
There was a problem hiding this comment.
I think an argument in favor of throwing an exception / requiring this in both cases is that the user might accidentally bypass the message box with a mis-click or mis-key. I say we should go forward with it. It's really easy to just enter "asdf" when testing here anyway.
| String name = cmd.getOptionValue("name"); | ||
| boolean convertToCdf = cmd.hasOption("convert-to-cdf"); | ||
|
|
||
| boolean validNameProvided = false; |
There was a problem hiding this comment.
I went ahead and redesigned this CLI logic so it handles corner cases more robustly. However, in doing so, I also found a critical flaw in this PR: the name that's entered and logged doesn't actually make it into the audit logs!
I think we need to make all this happen deeper in the app, i.e. immediately after this line:
Logger.info("Computer user name: %s", Utils.getUserName());
Also think it's probably fine if we don't prompt the user for their name when converting to CDF, but you could confirm with @chughes297 about this.
There was a problem hiding this comment.
Okay, I've made these changes: Convert to CDF doesn't require a name, and entering a blank name prevents tabulation.
artoonie
force-pushed
the
feature/issue-631_prompt-for-name
branch
from
2a1afde
to
2dbc9b7
Compare
# Conflicts: # src/main/java/network/brightspots/rcv/GuiConfigController.java
…o pass operator name validation.
| if (result.isPresent()) { | ||
| Logger.info("Name(s) of operators, as entered interactively: " + result.get()); | ||
| } else { | ||
| Logger.info("Operator(s) did not enter a name."); |
There was a problem hiding this comment.
Whoa whoa whoa, are you one of those? I'm impressed... 👽
I heard that truly 3lit3 engineers only use Colemak. |
Closes #631
In GUI:
In CLI:
CLI can still work interactively if
--nameis specified, but the audit log will show that the name wasn't interactively entered.Given the addition of the CLI argument, we needed the support of a framework to avoid clutter and stringent argument order.