Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[LINST] Remove '_date' only for Standard Format #6923

Merged

Conversation

zaliqarosli
Copy link
Contributor

@zaliqarosli zaliqarosli commented Aug 19, 2020

Brief summary of changes

This PR fixes the removal of '_date' by only allowing it to happen for Standard Date formats.

Currently, all date elements (standard, basic, and month/year) in linst instruments have their field names redefined as the substring appearing before '_date'. For example, if a date element is called 'enrolment_date_v2', the instrument field name becomes enrolment. The issue with this is that on saving the instrument, the save function will try to update a field called 'enrolment' in the instrument table which does not exist.

Now, the reason why this functionality existed in the first place is because the Standard Date format requires a field name ending with '_date'. Linst Standard Date formats are created by calling the NDB_BVL_Instrument class addDateElement() function which auto appends '_date' to the date element name. In order to avoid '_date' appearing twice, the NDB_BVL_Instrument_LINST class removes '_date' from the element name before calling addDateElement().

Basic Date and Month/Year formats call the NDB_Page class addBasicDate() function and the NDB_BVL_Instrument class addMonthYear) function respectively, both of which do not seem to auto append '_date' to the field name.

In this PR, if a Standard Date format field name does not already end with '_date' , a LorisException is thrown.

Testing instructions (if applicable)

  1. Add a Standard Date field to the $loris/project/instruments/bmi.linst. You can add to the end of the file:
date{@}completion_date{@}Date of completion{@}{@}{@}BasicDate
  1. Add the date field to the bmi SQL table. You can run in mysql:
alter table bmi add column completion_date date DEFAULT NULL;
  1. On main branch, fill out the date field and click 'Save Data' on the bmi instrument front-end for any candidate. You will get the 'Something went wrong.' page. Check your apache error log, and you should see errors
Column not found: 1054 Unknown column 'completion' in 'field list'
Update statement did not execute successfully

'Inspect' the HTML date element on the browser, and notice that the input name is 'completion' instead of 'completion_date'.
4. On this PR branch, repeat the above step and you will get no errors (except for maybe 'Error running scoring algorithm' which is a separate issue I think is unrelated to this PR)
5. On this PR branch still, modify the last line of the bmi.list file to be:

date{@}completion_date{@}Date of completion{@}{@}{@}

This now changes the date element from a Basic Date format to a Standard Date format. A standard date format date field requires a not_answered '_status' field so run in mysql:

alter table bmi add column completion_date_status enum('not_answered') default NULL;
  1. Refresh the instrument page, fill out the date field and click 'Save Data' again. You will get no errors. Inspect the HTML date element, and notice that the field name is completion_date and not completion_date_date
  2. After finishing testing, you can delete the columns you had added. Run in mysql:
alter table bmi drop column completion_date;
alter table bmi drop column completion_date_status;

@zaliqarosli zaliqarosli added Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Caveat for Existing Projects PR contains changes that might impact the code or accepted practices of current active projects Cleanup PR or issue introducing/requiring at least one clean-up operation labels Aug 19, 2020
@ridz1208
Copy link
Collaborator

@cmadjar @driusan @zaliqarosli

I just want to say that I will use this PR from now on as an example for what testing instructions look like !!!

@CamilleBeau CamilleBeau added the Passed Manual Tests PR has undergone proper testing by at least one peer label Nov 25, 2020
@zaliqarosli zaliqarosli closed this Dec 1, 2020
@zaliqarosli zaliqarosli reopened this Dec 1, 2020
@driusan driusan merged commit 434fd64 into aces:main Dec 21, 2020
driusan pushed a commit that referenced this pull request Dec 23, 2020
…7260)

These changes were made to reflect the #6923 which was issued to the main branch

Additional changes not in #6923:

    modified the "stripping" function to only remove _date if it is positioned at the end of the field name. For example if the user names their date field my_date_of_release_date, previously the function would strip the field name to my; now it would only strip the last _date to become my_date_of_release
    removed the check for isset($dateformat) since the code is already converting a null $dateformat to an empty string 2 lines above !
    reorganized the if statements to reduce over all complexity
    added an exception if the date format does not match any known formats
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
This fixes the removal of '_date' by only doing it for Standard Date formats.

Currently, all date elements (standard, basic, and month/year) in linst instruments have their field names redefined as the substring appearing before '_date'. For example, if a date element is called 'enrolment_date_v2', the instrument field name becomes enrolment. This results in the save function trying to update a field called 'enrolment' in the instrument table which does not exist.

The reason why this functionality existed is because the Standard Date format requires a field name ending with '_date'. Linst Standard Date formats are created by calling the NDB_BVL_Instrument class addDateElement() function which auto appends '_date' to the date element name. In order to avoid '_date' appearing twice, the NDB_BVL_Instrument_LINST class removes '_date' from the element name before calling addDateElement().

Basic Date and Month/Year formats call the NDB_Page class addBasicDate() function and the NDB_BVL_Instrument class addMonthYear) function respectively, both of which do not seem to auto append '_date' to the field name.

With this change, if a Standard Date format field name does not already end with '_date' , a LorisException is thrown.
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Caveat for Existing Projects PR contains changes that might impact the code or accepted practices of current active projects Cleanup PR or issue introducing/requiring at least one clean-up operation Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants