radiology-research / ispy2_mri

GUI to enter info on incoming MRI's for ISPY2 into database
GNU General Public License v3.0
0 stars 0 forks source link

handling of missing dates #8

Closed RossBoylan closed 9 months ago

RossBoylan commented 10 months ago

This issue concerns current behavior and the design of how to handle such things, which doesn't work well.

User Experience

After hitting save when there is a missing date a cryptic box pops up image The title is a truncation of "bad date" and the body is the result of using - as a separator for empty month day and year. The program proceeds once you hit OK, and saves NULL as the data.

Consequences

NULL dates, esp in the mri_date field, may be the cause of the problems noted in #7. There are 5418 records with null report_received_date and 2467 with null report_returned_date, but none with missing mri_date. 11276 total records. So it seems safe to conclude that null values are OK for the first 2 fields.

The user has no choice about whether to proceed, unless they never hit OK or terminate the program while it is waiting.

Why is there was only 1 warning when there were 3 blank fields? 2 of the fields are in the missing_fields list and so are automatic None with going through date().

Internals

isValid() returns true for BSmallDateWidget on a null date. Though I'm not sure I'm even calling the method right now.

BSmallDateWidget.date(), which is called by todb(), is where the action happens. If it can't convert to a valid date it uses QMessageBox.warning() to pop up the annoying warning shown above. Then it returns None. It traps all relevant errors, and raises no error on its own.

BSmallDateWidget has no ability to distinguish widgets that may be blank from others.

Potentially related to error handling in general #4

Desired Behavior

  1. Refuse to save data if values are invalid. In this context that means prohibit missing mri_date.
  2. Notify user if that happens.
  3. Should the method raise an error?
  4. I don't think putting the message box directly in the accessor is a good design. For one thing, it lacks key contextual information like the name of the field, and it bakes in logic for how to handle the problem.
RossBoylan commented 10 months ago

v0.0.7 deletes the message box in the accessor. Now the method just returns None without raising an error.

There is still nothing to enforce that a date field must be non-missing. Clearly report_received_date and report_returned_date can be missing (None in Python, NULL in the database) because many existing records have that.

However, all existing records have a real mri_date (after I deleted the ones I created and a couple of clearly stray, blank records). This suggests mri_date should be required.

RossBoylan commented 9 months ago

Fixed in v0.1.0. Null dates are still allowed, but I leave that to #7. If null dates don't cause #7, they are kind of "OK".