-
Notifications
You must be signed in to change notification settings - Fork 1
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
285 generate parameters from sql #288
Conversation
Add better error messages
and adding empty parameter sets
Bundle size differenceComparing this PR to
|
if (idx === params.length) { | ||
// Allow adding empty sets from JSON | ||
params.push({}); | ||
onDeleteParam(idx, 'this-is-a-hack-to-force-an-update'); | ||
} | ||
// Iterate through all valid keys | ||
for (const key of [...Object.keys(param), ...Object.keys(params[idx] || {})]) { | ||
if (param[key]) { | ||
// If we parsed the key from the JSON input, update it | ||
onUpdateParams(idx, key, param[key] || ''); | ||
} else { | ||
// If the key exists on the parameter object but not the JSON, delete it | ||
onDeleteParam(idx, key); | ||
} | ||
} | ||
}); | ||
while (params.length > editedParams.length) { | ||
onDeleteParam(editedParams.length, null); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole section of code is completely separate to the main issue, it just improves the behaviour of loading params from JSON. The previous implementation didn't allow for things like removing parameter sets, deleting individual keys from parameter sets, or adding empty parameter sets
if (parsedDraft) { | ||
// Backend expects this value to be null if unedited so don't load it from config | ||
parsedDraft.nextDueDatetime = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not associated with main issue but fixes a bug. The current config of nextDueDatetime
would always be sent alongside the rest of the data when sending an update to the config, when the backend sees this in the update query, it runs the notification. So the notification would always run on save whether you hit "Save" or "Save & Run"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
const { queryParams } = useQueryParamsState(); | ||
const { data: queriesData } = useNotificationQueries(queryParams); | ||
const selectedQuery = queriesData?.nodes.find(query => query.id === parameterQueryId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently I'm just pulling all the notification queries for filling up the autocomplete selector. This will be mostly fine until we get to large numbers of queries, we might want to paginate this selection a bit better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with grabing every thing for now.
I also didn't expect 500 parameter sets,so you never know...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work!
Happy to merge this, but also happy if you wanted to add something to smooth the creation of the parameter queries.
Would be nice if we didn't have to have a column called parameters
.
I think it would be better if just have a list of rows, each row becomes a parameter set? That would basically need to call pg_sql_query_as_json_rows
I think?
let mut sql_params: Vec<HashMap<String, serde_json::Value>> = serde_json::from_str(&sql_params_string) | ||
.map_err(|e| { | ||
NotificationServiceError::InternalError(format!( | ||
"Failed to parse notification sql parameters (expecting an array of params_string): {:?} - {}", | ||
e, params_string | ||
)) | ||
})?; | ||
all_params.append(&mut sql_params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be written like this instead.
I'm not sure if one is strictly better than the other but in general I try to only make this mutable if they have to be.
let mut sql_params: Vec<HashMap<String, serde_json::Value>> = serde_json::from_str(&sql_params_string) | |
.map_err(|e| { | |
NotificationServiceError::InternalError(format!( | |
"Failed to parse notification sql parameters (expecting an array of params_string): {:?} - {}", | |
e, params_string | |
)) | |
})?; | |
all_params.append(&mut sql_params); | |
let sql_params: Vec<HashMap<String, serde_json::Value>> = serde_json::from_str(&sql_params_string) | |
.map_err(|e| { | |
NotificationServiceError::InternalError(format!( | |
"Failed to parse notification sql parameters (expecting an array of params_string): {:?} - {}", | |
e, params_string | |
)) | |
})?; | |
all_params.extend(sql_params); |
Weird to me that append requires &mut
, but I guess it needs the same datatype...
I think it's fine how it is but wanted to check why i needed to be mut
ctx: &ServiceContext, | ||
parameter_query_id: &String, | ||
) -> Result<String, NotificationServiceError> { | ||
// TODO: Maybe split these to a new database table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite a bit of work, and I'm not sure it's needed but could be handy, or maybe add a query type so we can filter?
|
||
let parameters = match parsed_results[0].get("parameters") { | ||
None => return Err(NotificationServiceError::InternalError(format!( | ||
"No 'parameters' column found in query results - {}", parameter_query_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we did create a new method we could automatically run something like SELECT json_agg(q.*) as parameters from (<THEQUERY>) as q
?
Then we wouldn't have to the restrictions about the column naming and parameter queries would be a bit easier to test?
@@ -124,5 +126,8 @@ pub fn generate( | |||
// Note: We usually reset the next check datetime in case the schedule has changed, or something needs to be recalculated | |||
new_notification_config_row.next_due_datetime = next_due_datetime; | |||
|
|||
// We might want to clear out the parameter_query_id so it's allowed to be None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we're just seeing to empty string now? It seems to be working??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more a comment about why it's not using the if let Some(X)
check we use for other values. Since that doesn't let you un-set it back to None
const { queryParams } = useQueryParamsState(); | ||
const { data: queriesData } = useNotificationQueries(queryParams); | ||
const selectedQuery = queriesData?.nodes.find(query => query.id === parameterQueryId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with grabing every thing for now.
I also didn't expect 500 parameter sets,so you never know...
// If we parsed the key from the JSON input, update it | ||
onUpdateParams(idx, key, param[key] || ''); | ||
} else { | ||
// If the key exists on the parameter object but not the JSON, delete it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate all these comments!
if (parsedDraft) { | ||
// Backend expects this value to be null if unedited so don't load it from config | ||
parsedDraft.nextDueDatetime = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
Nice! Was thinking that after I added that comment, isn't it already json... |
Closes #285
π©π»βπ» What does this PR do?
Added a new section to notifications to add an SQL query. Currently these are the same queries used for notifications
A parameter query is expected to return a single row with a column "parameters", as a JSON array in the same format as our JSON parameter editing in the UIA parameter query is expected to return a column for each parameter, with each row being used as a parameter set
When you run the notification, a new event should generate for each defined parameter from the query result (In this example I'm just listing all the tables in my local database and the id of the first entry (There's a number of failures because I don't have data in all my tables)
This should work alongside regularly defined parameters! The list of query generated parameters and the list of user provided parameters will be merged when creating events.
In addition I've made it possible for a notification to be defined with no user defined parameters, so a notification can be created without any parameter sets (and pull exclusively from query parameters). I've added an additional "create" button for adding a new parameter when none exists already
π§ͺ How has/should this change been tested?
Tested locally running the queries screenshotted above β¬οΈ
π Any notes for the reviewer?
This is working, but there's definitely some improvements we could make. Currently I'm just using notification queries for fetching parameters, splitting those out to their own defined table similar to sql recipient lists is an obvious improvement.
This is also my first time writing rust so let me know if I've done anything silly there π
π Documentation