[instrument_builder] Fix Date element corruption in LINST files when load/saving

[ridz1208]

Brief summary of changes

This PR fixes multiple issues with Date elements in LINST files that have existed for several previous releases. Due to the age of these issues, some backwards compatibility logic was necessary for proper functioning of existing projects.

1. when adding a date field in the instrument builder, If the date format was not manually changed on the frontend, the default format was simply an empty string ''. If however a user selected a date format and then re-selected the default standard date, the date format was saved as Date. Standard dates in existing instruments are saved as either Date or '' currently due to this bug and thus both versions are currently!! Prospectively only Date will be saved in .linst files and a deprecation message was added to slowly modify all empty empty or null date formats to Date
2. When loading a linst in the builder, the dateformat was never being loaded and thus when saving an instrument with a date after loading it, the date would save as undefined (see linked issue for more details )
3. There was a confusion regarding MinDate and MaxDate in some instances they were handled as regular date formats YYYY-MM-DD in other places they were handled as minimum year and maximum year. There was NO evidence of full dates being ever required in the code... so I converted all MinDate occurrences to MinYear and all MaxDate occurrences to MaxYear to avoid further confusion and removed all code handling splitting the dates to extract the year element from both loading and saving.

Testing instructions (if applicable)

1. Follow instructions and examples in linked issue

Link(s) to related issue(s)

• Resolves [instrument_builder] Loading existing instrument corrupts Date element #6669
• This PR replaces [instrument_builder] Loading existing instrument corrupts Date element #7147

[ridz1208]

@zaliqarosli all yours, read description before testing

[maltheism]

@ridz1208 loading of linst files with dates that are in format of "Basic Date" and "Month Year" will have the incorrect Date Format set when clicking the edit button. I fixed the foregoing in the PR that's on main if you want to see what I did. I'm still confused why that PR code won't work for this branch.

Also observing the output of lisnt containing select{@}date1test_date_status{@}{@}NULL=>''{-}'not_answered'=>'Not Answered' after the default date type line of the generated linst file.

Btw @ridz1208 , thanks for fixing it. I find the this module & bug so confusing lol.

[ridz1208]

@maltheism thanks for the feedback, I will look into the basicdate and monthyear stuff. I think you were on the right track, the only issue with your fix was you changung the "date" type to one of 3 "date", "monthyear" and "basicdate". the type should stay date, it's the date format that had to be modified according to the type.

I'll try to fix this and you can take a look at it after

[ridz1208]

@maltheism i stole your fix :)
@zaliqarosli all yours

[zaliqarosli]

i think this is fine. just keep in mind this PR https://github.com/aces/Loris/pull/6923/files that has conflicting code, and which will be merged first. i'll test now and report back.

edit: PR 6923 has already been merged to main. i like the way the logic is organized here though. resolving the conflict will be necessary at some point when branches merge

[driusan]

I don't think this description is clear enough for a user who sees it to know what action they need to take to update their instrument. In particular, I'm not sure how to interpret "Make sure to specify the format as Date in all your .linst files as the 6th parameter of you date entries where applicable."

[driusan]

Also what's a "standard date" and why is it being removed? We haven't had any backwards incompatible changes to the LINST format and it seems unwise to start given that this is already backwards compatible(?).

[zaliqarosli]

there's a small side effect whereby '_date' is getting appended again to standard dates with 'Date' date format that doesn't happen on 23.0-release.

linst1:

table{@}test_date_5_bugfix
title{@}Test date 5 bugfix
date{@}Date_taken{@}Date of Administration{@}2006{@}2012
static{@}Candidate_Age{@}Candidate Age (Months)
static{@}Window_Difference{@}Window Difference (+/- Days)
select{@}Examiner{@}Examiner{@}NULL=>''
date{@}without_limit_date{@}Date without limits{@}2010{@}2019{@}Date
select{@}without_limit_date_status{@}{@}NULL=>''{-}'not_answered'=>'Not Answered'
date{@}with_limit_date{@}Date with limits{@}2010{@}2030{@}Date
select{@}with_limit_date_status{@}{@}NULL=>''{-}'not_answered'=>'Not Answered'

linst2:

table{@}test_date_5_bugfix_again_again
title{@}Test date 5 bugfix
date{@}Date_taken{@}Date of Administration{@}2006{@}2012
static{@}Candidate_Age{@}Candidate Age (Months)
static{@}Window_Difference{@}Window Difference (+/- Days)
select{@}Examiner{@}Examiner{@}NULL=>''
date{@}without_limit_date_date{@}Date without limits{@}2010{@}2019{@}Date
select{@}without_limit_date_date_status{@}{@}NULL=>''{-}'not_answered'=>'Not Answered'
date{@}with_limit_date_date{@}Date with limits{@}2010{@}2030{@}Date
select{@}with_limit_date_date_status{@}{@}NULL=>''{-}'not_answered'=>'Not Answered'

this was reported in #6863 and alizee attempted to fix that in #7147

[maltheism]

@zaliqarosli do you know if that added select line in your comment example should be included? I think it shouldn't be and because its not there under the date line that's under title line and similar that date line doesn't even have the _date.

[ridz1208]

@zaliqarosli i will look into it, thanks

[zaliqarosli]

@maltheism Standard dates (with 'Date' format) should have the 'Not Answered' select field. BasicDates should not. I guess Date_taken is technically a basic date element. FYI Date_taken is a metadata field that the user doesn't add themselves on the instrument_builder

[maltheism]

@zaliqarosli if Date_taken is a basic date element and doesn't require _date then I'm wondering if _date isn't really required at all.

Thanks for the clarification of the 'Not Answered' part and makes me realize my PR needs that because I thought it wasn't something that should be there.

[zaliqarosli]

hey @maltheism whether "_date" is required or not is a separate issue that shouldn't be discussed in this PR. let's keep things separate. thanks

[maltheism]

@zaliqarosli I'll copy it over to my PR so not forgotten.

[ridz1208]

@driusan removed deprecation warning
@zaliqarosli handled the double _date case by removing it on loading.

[zaliqarosli]

all good as long as we keep in mind the code conflict that will happen with #6923 at some point

[ridz1208]

@zaliqarosli why was 6923 sent to main not 23 ? isnt it a bugfix technically ?

[zaliqarosli]

@ridz1208 no reason at all other than miscommunication probably. i wasn't aware we were still working on 23.0-release so i just sent it to the main branch when i made the PR because in my mind that was the latest branch. and then the PR got approved and noone told me to change branches! 🤷‍♀️ but yess, it would be a bugfix

[driusan]

So how should we deal with this? #6923 is already merged into main. Should I wait until there's a PR equivalent of #6923 to merge into this branch before merging this or should I merge this and then someone can backport #6923 and deal with the conflicts?

[zaliqarosli]

i think maybe issuing a duplicate PR would be easier than dealing with the conflict but up to @ridz1208

[ridz1208]

@zaliqarosli @driusan I'll make another PR for that today. lets get this merged