-
Notifications
You must be signed in to change notification settings - Fork 175
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
[Instrument] Add support for jsonData instrument in clearInstrument function #7484
Conversation
Tested this on MCNRR and it all looks good. |
$columns = $db->pselect( | ||
"SELECT COLUMN_NAME FROM information_schema.columns | ||
if ($this->jsonData) { | ||
$db->update('flag', ['Data'=>null], ['CommentID' => $this->commentID]); |
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.
Shouldn't this update be unconditional? The Data
column in the flag table exists regardless of whether jsonData is true or not, and the update when saving data in _save
isn't conditional.
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.
You are correct. Ma bad
@driusan changes done |
Re-tested after the changes with MCNRR and it works alright. |
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 can settle for the downgrade from "good" to "alright" if it means it's ready to merge.
@driusan I think we should merge before @GabrielPelletier takes it from "alright" to "its not bad" |
I like a bit of variation; it makes for richer github Conversations, literature-wise |
Brief summary of changes
clearInstrument
Function gives an error when the instrument has jsonData enabled. this fix adds support for clearInstrument by clear the data from the proper locationPS, make sure to ignore white spaces changes as most changes here are just indentation
Testing instructions (if applicable)
Link(s) to related issue(s)