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

[configuration] Add date_format as DataType in ConfigSettings #6719

Merged

Conversation

jesscall
Copy link
Contributor

@jesscall jesscall commented Jun 10, 2020

Brief summary of changes

Quick fix for candidate parameter tabs Date of Birth and Date of Death. Altering the DOB format or the DOD format fields in the configuration module to either YM or Ym should result in the dates in cand param being saved with YYYY-MM-01 as the day of the month rather than whatever the user entered.

Edit:
This PR now adds a new dataType to ConfigSettings: date_format. Fields such as DOB and DOD format will now have a dropdown in which users that select what format they prefer. This should also resolve the ambiguity in candidate parameters that is discussed in the thread below.

Testing instructions (if applicable)

  1. See test plan steps 35 & 40

Link(s) to related issue(s)

@jesscall jesscall added Category: Bug PR or issue that aims to report or fix a bug 23.0.0-testing labels Jun 10, 2020
@jesscall jesscall requested a review from CamilleBeau June 10, 2020 18:55
@driusan
Copy link
Collaborator

driusan commented Jun 11, 2020

@jesscall this is failing Travis

@ridz1208
Copy link
Collaborator

@jesscall The way I understand it, the casing of the YMD letters actually dictates the formate of dates so I don't think it's a good idea to make all combinations of Ym YM yM ym do the same thing, just in case we decide to support more formats later on.

I think the proper fix to the reported issue would be to throw a configuration error in the config module if you choose a date different then the only 2 supported formats OR to change that configuration field to a drop down and progressively add formats to it as we move forward.

@ridz1208 ridz1208 changed the title [canidate_parameters] fix saving of DOB / DOD date format according to config [candidate_parameters] fix saving of DOB / DOD date format according to config Jun 11, 2020
@maltheism
Copy link
Member

@ridz1208 @jesscall something like https://www.php.net/manual/en/datetime.format.php might be useful for converting to the correct format.

@ridz1208 ridz1208 added this to the 23.0.1 milestone Jun 12, 2020
@jesscall jesscall changed the title [candidate_parameters] fix saving of DOB / DOD date format according to config [configuration] Add date_format as DataType in ConfigSettings Jun 16, 2020
@jesscall jesscall added the Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects label Jun 16, 2020
@jesscall
Copy link
Contributor Author

@CamilleBeau ready for review

[Notes]:

  • The PR has gone in a different direction. It now adds a dropdown to configuration module where the user can select a format from a pre-determined list.
  • I've hard-coded this list as there are only 2 formats we support at the moment, however I wouldn't mind creating a table for this since it is possible the list grows over time.
  • The caveat for projects is that the date format entered in the text field may not match the dropdown options which will lead to some issues. On upgrade projects should double check and amend where applicable.

[Testing]:

  • Please test the configuration module DOB & DOD format fields.
  • Also test if the original bug is resolved.

@CamilleBeau
Copy link
Contributor

Error given when sourcing rb while in this branch:
image

@CamilleBeau CamilleBeau added the State: Needs work PR awaiting additional work by the author to proceed label Jun 17, 2020
@ridz1208 ridz1208 modified the milestones: 23.0.1, 23.0.2 Jun 17, 2020
@johnsaigle
Copy link
Contributor

johnsaigle commented Jun 17, 2020

In general the way we display the date shouldn't impact what is saved in the database. The logic should be decoupled. A user can enter a year, month and day on the front-end and we can drop the day value if necessary when it goes into the database. When we display the date, we can show or hide to whatever resolution we want.

As @maltheism suggested, this would be easier with DateTime objects. We can use them to validate date formats or easily replace just the date value. It's a more robust way than working with strings.


There's also the config setting dateDisplayFormat. I think it should also be changed in the SQL for consistency along with any other date config settings.

Alternatively - I'm not sure if it's important for the DoD and DoB formats to be different. Could they both instead use dateDisplayFormat?

@johnsaigle johnsaigle added the Language: SQL PR or issue that update SQL code label Jun 17, 2020
@ridz1208
Copy link
Collaborator

@johnsaigle the way we save the data does impact the display. Users get confused when they can select a day but it defaults to 01 after saving. The display should clearly show that you are selecting a whole month at a time

@maltheism
Copy link
Member

maltheism commented Jun 17, 2020

@ridz1208 maybe the user's "format" of the date should be saved in the database and used for the standardized date in the database to be reverted to the correct format for the user on the frontened.

@johnsaigle
Copy link
Contributor

@ridz1208 I think we're saying the same thing. We should save whatever the user enters but display it with either the day chopped off or with -01. Alternatively if the day is never needed then I think the issue is with the input fields. We shouldn't prompt for a day if it won't be used.

@jesscall
Copy link
Contributor Author

jesscall commented Jun 18, 2020

@johnsaigle the issue with saving the full date in the DB and then only displaying the day as 01 is that it makes anonymization / identification of a candidate a concern when it comes to data releases.

Re: not having a day field displayed at all:
Most times, data entry staff is just inputting a date of birth that was recorded on paper forms and might cause confusion when entering into LORIS. Also not sure how aware data entry personnel are of the config options or desire for a study to have anonymized data.

I'm open to hearing your opinions as well.
@ridz1208 @maltheism

@maltheism
Copy link
Member

maltheism commented Jun 18, 2020

@jesscall @johnsaigle @ridz1208 I think having the actual day is important in keeping things consistent. Although, I doubt having the correct day of the month when a person died compared to a few days off is really valuable. It's probably better to not fake a day from the software side of LORIS.

Why not just force the configuration module for DOB & DOD Format to always have "d" in the input field and an input error is displayed if otherwise. You could use regex.

Maybe there should be a field for notes and where if the day isn't known the study admin would just add a note.

@ridz1208
Copy link
Collaborator

@maltheism those are ethics regulated requirements, some projects are not allowed to save the day of the date of birth because it's identifiable

@maltheism
Copy link
Member

maltheism commented Jun 18, 2020

@ridz1208 hmm if that's the case maybe there should be two more date columns in database.
Ex. actual_dod, actual_dob, ethics_dod, ethics_dob

Someone that's born in the 1st of whatever month and dies on the 1st of whatever month. Wouldn't be happy if they were in the ethical study.

@ridz1208
Copy link
Collaborator

@maltheism the idea is that we can't store the actual values in the database just like we can't store names and addresses

@maltheism
Copy link
Member

maltheism commented Jun 18, 2020

@ridz1208 the ethics_dod or ethics_dob could be scrambled and the actual_dob & actual_dod would be left null depending on if it's an ethical study.

Edit: Maybe jess's solution is best after all if the person managing the study doesn't care about the date being accurate.

@jesscall
Copy link
Contributor Author

@CamilleBeau I suspect the data truncation is happening because of the note i left about existing projects having different values than the preset formats. Should be solved by running the last 4 queries in the patch submitted.

@jesscall jesscall removed the State: Needs work PR awaiting additional work by the author to proceed label Jun 18, 2020
@ridz1208 ridz1208 removed this from the 23.0.2 milestone Jun 19, 2020
@ridz1208 ridz1208 changed the base branch from 23.0-release to master June 19, 2020 16:00
@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 22, 2020
@jesscall jesscall force-pushed the 2020_06_10_candidate_parameter_DOB_DOD_date_format branch from e223309 to ebf0301 Compare June 30, 2020 18:49
@jesscall jesscall removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 30, 2020
@ridz1208
Copy link
Collaborator

ridz1208 commented Jul 2, 2020

@jesscall travis

@CamilleBeau
Copy link
Contributor

Getting ERROR 1265 (01000) at line 2402: Data truncated for column 'DataType' at row 1 when sourcing raisinbread

@CamilleBeau CamilleBeau added the State: Needs work PR awaiting additional work by the author to proceed label Jul 6, 2020
@jesscall jesscall removed the State: Needs work PR awaiting additional work by the author to proceed label Jul 6, 2020
CamilleBeau
CamilleBeau previously approved these changes Jul 6, 2020
@jesscall jesscall changed the base branch from master to main July 28, 2020 15:07
@@ -61,6 +61,12 @@ class Configuration extends \NDB_Form
$scan_types[$val] = $val;
}

$date_format = [
'' => '',
'Ymd' => 'Ymd',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be a good idea to show a more descriptive label for these on the frontend since it's a dropdown now?

Like 2015-12-29 (Ymd) and 2015-12 (Ym)?

(Date is arbitrary, just something that's unambiguous with the day of the month > 12.. I'm assuming m is numeric month, I haven't looked into it, but it would avoid that type of confusion for users to provide an example in the dropdown label..)

@driusan driusan merged commit 331d1fa into aces:main Aug 11, 2020
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 18, 2020
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 15, 2021
)

Add "date_format" config format type for DoB and DoD, which configures whether day of month is included in date.

    Resolves aces#6678
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
)

Add "date_format" config format type for DoB and DoD, which configures whether day of month is included in date.

    Resolves aces#6678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug PR or issue that aims to report or fix a bug Language: SQL PR or issue that update SQL code Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[candidate_parameters] Date format does not match dates specified in config
6 participants