Skip to content
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

Review by records instead of subject_id & form_id "rv_row" #137

Open
wants to merge 36 commits into
base: dev
Choose a base branch
from

Conversation

jthompson-arcus
Copy link
Collaborator

Still lead up to addressing #99. Review process needs to depend on records being reviewed, not the review_row it currently does.

Merge branch 'jt-113-simplify_review_process' into jt-99-review_by_row

# Conflicts:
#	R/fct_SQLite.R
#	man/db_save_review.Rd
#	tests/testthat/test-fct_SQLite.R
#	tests/testthat/test-mod_review_forms.R
Merge branch 'dev' into jt-99-review_by_row

# Conflicts:
#	R/mod_review_forms.R
#	tests/testthat/test-mod_review_forms.R
R/fct_SQLite.R Outdated Show resolved Hide resolved
R/fct_SQLite.R Outdated Show resolved Hide resolved
@jthompson-arcus jthompson-arcus marked this pull request as ready for review November 20, 2024 20:28
R/fct_SQLite.R Outdated Show resolved Hide resolved
R/mod_review_forms.R Outdated Show resolved Hide resolved
R/mod_review_forms.R Outdated Show resolved Hide resolved
R/mod_review_forms.R Outdated Show resolved Hide resolved
R/fct_SQLite.R Outdated Show resolved Hide resolved
R/fct_SQLite.R Outdated Show resolved Hide resolved
R/fct_SQLite.R Outdated Show resolved Hide resolved
R/mod_review_forms.R Outdated Show resolved Hide resolved
R/mod_review_forms.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@LDSamson LDSamson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor remarks. One change confuses me a bit though, see comments

@LDSamson
Copy link
Collaborator

@jthompson-arcus I wanted to make some minor changes and then merge this branch, but I realized that bug #108 was reintroduced in this PR when testing interactively, and our tests did not capture it properly.
image

I updated the tests so that they would now fail as expected, and have a look at it at a later moment.

Comment on lines 19 to 21
"export": {

"test-review_save_error": false
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is failing but I changed the snapshot manually to what it should show, for now

# Within a form, only items with a changed review state are updated and
# contain the new (current) time stamp.
dplyr::filter(timestamp == review_records$timestamp[1])
if(isTRUE(all.equal(review_records_db, review_records, check.attributes = FALSE))){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this statement now fails: the data frame review_records contains all possible item updates, but in the database only records with a changed review status will be updated. Thus, this statement does not always return TRUE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants