Add optional overwrite parameter to CreateJob and CreateJobs
#916
Conversation
plugins/Jobs.cpp
Outdated
| @@ -402,6 +402,8 @@ void BedrockJobsCommand::process(SQLite& db) { | |||
| // - jobPriority - High priorities go first (optional, default 500) | |||
| // - unique - if true, it will check that no other job with this name already exists, if it does it will | |||
| // return that jobID | |||
| // - overwrite - if true, and unique is true, it will overwrite the existing job with the new jobs data | |||
There was a problem hiding this comment.
| // - overwrite - if true, and unique is true, it will overwrite the existing job with the new jobs data | |
| // - overwrite - Only applicable when submit is is true. When set to true it will overwrite the existing job with the new jobs data |
plugins/Jobs.cpp
Outdated
| "WHERE jobID = " + SQ(updateJobID) + ";")) | ||
| { | ||
| STHROW("502 update query failed"); | ||
| if (!SContains(job, "overwrite") || job["overwrite"] == "true") { |
There was a problem hiding this comment.
I think you also want to check if overwrite is == and treat it as true too?
There was a problem hiding this comment.
I'm not quite following. What would we check if overwrite is == to?
There was a problem hiding this comment.
Damn sorry, I mean is equal to ''
There was a problem hiding this comment.
Ah good catch, will add that
| // Update the existing job. | ||
| if(!db.writeIdempotent("UPDATE jobs SET " | ||
| "repeat = " + SQ(SToUpper(job["repeat"])) + ", " + | ||
| "data = JSON_PATCH(data, " + safeData + "), " + |
There was a problem hiding this comment.
hmmm you sure about PATCH? Don't we want to replace the whole data?
There was a problem hiding this comment.
This is the same query as before, just indented. I agree it probably should not be a PATCH, but I don't want to change existing behavior (in case any relies on this)
|
Updated |
|
All looks good, I did not hit the merge button because it seems real tests have not been done yet... feel free to merge when you do. |
|
Ah, this was tested with https://github.com/Expensify/Web-Expensify/pull/29514, going to hit the merge button |
@iwiznia can you review please?
Part of https://github.com/Expensify/Expensify/issues/143805
This adds an optional parameter to
CreateJobandCreateJobswhere we will not overwrite any data when when attempting to create a unique job that already exists. This optional parameter is assumed to betruewhen not included to not break any existing behaviorTests
Added a new automated test, will be further tested in followup prs
Tested in a full flow with https://github.com/Expensify/Web-Expensify/pull/29514