fix: store user info early #5729
Conversation
aiida/cmdline/commands/cmd_setup.py
Outdated
| @@ -54,6 +54,9 @@ def setup( | |||
| from aiida import orm | |||
| from aiida.manage.configuration import get_config | |||
|
|
|||
| # store default user settings so user does not have to re-enter them | |||
| config = _store_default_user_settings(non_interactive, email, first_name, last_name, institution) | |||
There was a problem hiding this comment.
The config is being fetched in this function, but then it is retrieved once again on line 83. I would propose to get the config once in this function, and pass it as the first argument to _store_default_user_settings.
Better yet, the context is passed in the ctx argument passed to the click command. I would simply use this and get rid of the get_config altogether. So simply call
_store_default_user_settings(ctx.obj.config, email, first_name, last_name, institution)and also use ctx.obj.config elsewhere in the command.
Also, please add a test because this is currently failing. You are passing in non_interactive but the function doesn't accept it.
There was a problem hiding this comment.
Thanks for this suggestions, I didn't know the config was automatically shipped in the context.
The function is tested by the setup/quicksetup tests.
If desired, I could still add a test to check whether the info is stored even if quicksetup fails
There was a problem hiding this comment.
If desired, I could still add a test to check whether the info is stored even if quicksetup fails
Yes, this is what I meant, would be great if you could add it. It should be relatively straightforward to test it. Just pass non-sensical values for db-connection using the empty_config fixture to guarantee an empty config, and then check that the relevant autofill keys in the Config are set.
There was a problem hiding this comment.
Thanks @ltalirz . I had some suggestions to improve the test, but it affected lines not covered by the PR, so instead I added a commit, hope you don't mind. Let me know if you are happy with the change, then I will merge this PR.
ltalirz
force-pushed
the
fix/store-default-user-settings
branch
from
52c7959
to
9a4d581
Compare
ltalirz
force-pushed
the
fix/store-default-user-settings
branch
from
732ef05
to
28f0a38
Compare
`verdi setup` and `verdi quicksetup` store information about the user, such as their name, email, and institution, but they only do so when the setup finishes successfully. This is tedious when setup fails as the user is forced to re-enter this information anew for every try. Here we simply make sure to store this information early.
* Use a dedicated separate test * Check the autofill is not already set before the CLI call
sphuber
force-pushed
the
fix/store-default-user-settings
branch
from
28f0a38
to
4ba9766
Compare
|
Note that I rebased the branch to make it up-to-date, but github seems to misattribute the author to me. If I look on the CLI, |
|
All good, please feel free to merge |
|
Thanks a lot @ltalirz |
verdi setupandverdi quicksetupstore information about the user, such as their name, email, and institution, but they only do so when the setup finishes successfully.This is tedious when setup fails as the user is forced to re-enter this information anew for every try. Here we simply make sure to store this information early.