civiCRM email summary report etl #856
Conversation
|
Hi @de-code, I am not pretty sure about the authentication key handling in the code, it would be good to look at it together when you find time today or tomorrow before deploying. |
|
|
||
|
|
||
| def get_civicrm_credential(key_name: str): | ||
| secret_file = get_env_var_or_use_default(CIVICRM_KEY_SECRET_FILE_PATH_ENV, "") |
There was a problem hiding this comment.
I guess by this time, the environment variable is required?
Is that the same configuration that we use for the current Civi API access or is that something new?
| def get_civicrm_credential(key_name: str): | ||
| if key_name == "api_key": | ||
| return read_file_content( | ||
| get_env_var_or_use_default(CIVICRM_API_KEY_FILE_PATH_ENV, "") |
There was a problem hiding this comment.
Could we make it so the environment variable is required? It would make error messages more informative.
| return data_configuration | ||
|
|
||
|
|
||
| def get_civicrm_credential(key_name: str): |
There was a problem hiding this comment.
minor: Maybe rather than passing in the key name, we could just pass in the env variable name?
Otherwise at the moment it isn't clear what the accepted key names are.
There was a problem hiding this comment.
Or alternatively these could just be two separate functions. They don't really share any code.
| return response.json() | ||
|
|
||
|
|
||
| def format_email_report( |
There was a problem hiding this comment.
minor: usually I would associate a format function with a text output. Perhaps another name could make it more clear? Maybe transform? And maybe it could say from what to what format do we transform it?
|
|
||
| LOGGER = logging.getLogger(__name__) | ||
|
|
||
| CIVICRM_KEY_SECRET_FILE_PATH_ENV = "CIVICRM_KEY_SECRET_FILE_PATH" |
There was a problem hiding this comment.
I believe this can now be removed? (And any other related mention in other files)
|
|
||
|
|
||
| def get_civicrm_credential(env_var_name: str): | ||
| if env_var_name == "CIVICRM_API_KEY_FILE_PATH_ENV": |
There was a problem hiding this comment.
You no longer need this condition, now that you pass in the env var name.
You would just use the env var name.
There was a problem hiding this comment.
You mean creating two different function ? Because I did not get the point of not using condition (if statement)
There was a problem hiding this comment.
Creating two separate functions was the alternative...
Buf if we pass in the environment variable name to the function then we can just use it, e.g.:
return read_file_content(get_required_env_var(env_var_name))There was a problem hiding this comment.
Ah yes sorry, my mind is a bit full atm.
https://github.com/elifesciences/data-hub-issues/issues/332