-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix TypeError for boolean opt-out fields when optout_col_values contains non-boolean values #142
Conversation
Some tests were being skipped by pytest because the class names did not end in *Tests
Otherwise newer versions are loaded that require sphinx >= 5.0
@RudolfCardinal as discussed, the script now aborts with ValueError (as with the other configuration checks) if there are no valid values for |
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.
Looks good! I did wonder about the part in validate_optouts()
in which the check for completely absent values in optout_col_values
only occurs if the column is boolean (which is when it may just have been trimmed). I thought: should that check apply regardless of column type? But then I thought: this table might be a list of opt-out PIDs (e.g. a table with a single INT column), in which case this may be absolutely correct -- you don't care what the opt-out value is, because anything counts as an opt-out, and that's what you code correctly does. So I think spot on as it is.
Fixes #140 so that for boolean opt-out fields, non-boolean values are dropped from
optout_col_values
in the anonymisation config file.@RudolfCardinal A side-effect of this is that if there are no valid values for
optout_col_values
, all patient ids will be considered as opted out. Is this OK?In adding some tests, I've also added support for database-level testing from pytest, initially with SQLite and MySQL. It may well work if you pass a non-MySQL url to pytest but I've only tested with MySQL. This will change when I add this functionality to #141 and we support more backends.
I've also changed the names of some other test classes, which were previously being ignored by pytest.