Simplify review process #135
Conversation
|
This PR has two goals:
|
R/fct_SQLite.R
Outdated
| # This will trigger before any UPDATEs happen on all_review_data. Instead of | ||
| # allowing 'id' to be updated, it will throw an error. | ||
| rs <- DBI::dbSendStatement(con, paste( | ||
| "CREATE TRIGGER all_review_data_id_update_trigger", | ||
| "BEFORE UPDATE OF id ON all_review_data", | ||
| "BEGIN", | ||
| "SELECT RAISE(FAIL, 'all_review_data.id is read only');", | ||
| "END" | ||
| )) | ||
| DBI::dbClearResult(rs) |
There was a problem hiding this comment.
I tried to test this behavior with a test database. Could it be that these are not updated yet? Also, is it possible to capture this in a test? It is quite important that this does not go wrong.
There was a problem hiding this comment.
Should this behavior be extended to all idx_cols?
There was a problem hiding this comment.
Good question. I feel like the id column is the most important since it links the review data to the audit/log table.
I am trying to thing if there would be a moment that one of the idx_cols/common_vars gets updated.
What would happen if this occurs for the event_name (e.g. from visit 1 to visit 2, although I think that would be rare)? Would that count as a completely new row with a new id?
There was a problem hiding this comment.
Well with the construction as it stands now (and how it functioned previously I believe) this would get input to the review data base as a new record and a warning would be triggered indicating "items were not found in the updated dataset". See update_review_data().
| dplyr::collect() | ||
| if(nrow(new_review_rows) == 0){return( | ||
| warning("Review state unaltered. No review will be saved.") | ||
| )} | ||
| new_review_rows <- new_review_rows |> | ||
| db_slice_rows(slice_vars = c("timestamp", "edit_date_time"), group_vars = common_vars) |> |
There was a problem hiding this comment.
Note that this is the only place where we are using the common_vars; maybe the function argument is then not needed anymore?
Also, for the SQL statement below: what happens if, for example, row 20 is saved by someone else previously and then the entire form will be saved by me. Will the name of the reviewer of row 20 be my name or still from the other person?
There was a problem hiding this comment.
The tests you added in #134 are important in capturing this behavior. I may go ahead and add them to this PR so that behavior is more clearly captured and shown in the tests.
|
@LDSamson let me know what you think of these changes. I think adding a test could be good but will need to rework |
|
@jthompson-arcus I think it looks okay as far as I can see but it is a bit difficult to see the differences between the changes I made (in the currently open PR on main) and this one, for the tests. I will have a closer look tomorrow. |
@LDSamson I copied from and compared to #134. Only difference I could tell was ordering. |
…l#long-function-calls, improve db_add_log docs, add function argument check to db_add_log
There was a problem hiding this comment.
@LDSamson let me know what you think of these changes. I think adding a test could be good but will need to rework
db_add_primary_key()anddb_add_log()to not be soall_review_datafocused.
It would be great to add tests indeed since this is a vital part of functioning of the application. But it looks good and the tests can be added in a follow-up PR, it all looks like it is working as intended now.
Note that I added some minor changes, mainly to adhere better to the tidyverse style guide.
Addresses part of #113. Follows PR #115.