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

Replace Center_name in mri_protocol with CenterID from psc #7525

Merged
merged 5 commits into from
Oct 6, 2021

Conversation

vietdhoang
Copy link
Contributor

@vietdhoang vietdhoang commented Aug 10, 2021

Summary

From #646: Currently, when an mri_protocol is not being restricted for a specify Center_name, users enter either ZZZZ or AAAA and it is understood that an mri_protocol with Center_name ZZZZ or AAAA is valid for all centres' scans.

This PR fixes this 'hack' by replacing the Center_name field with CenterID from the psc table. CenterID is an integer and a unique indentifier for each center. When an mri_protocl is not restricted to a specific center, the default value is NULL (instead of ZZZZ/AAAA).

Testing instructions

It is recommended that everything should be tested using the RaisinBread dataset. To populate your database with RaisinBread data, run the script tools/raisinbread_refresh.php. WARNING: any previous data will be erased.

  1. First, we will check if the SQL patch works. Source the new SQL patch:
source SQL/New_patches/2021-07-29_modify_center_name_in_mri_protocol.sql;
  1. Verify that the Center_name field has been replaced by CenterID and that the default values (rows that used to contain AAAA or ZZZZ) are NULL.
SELECT * FROM mri_protocol;
  1. Next, we will have to check if the changes made in 0000-00-00-schema.sql work. First, reset the RaisinBread dataset using tools/raisinbread_refresh.php. This command will have to be done on a different branch where 0000-00-00-schema.sql has not been changed yet. Verify that the refreshed mri_protocol table no longer has the CenterID field from step 2.
  2. Next, switch to the branch containing all the changes made by this PR. Run the raisinbread_refresh.php again. This script will source the 0000-00-00-schema.sql file that has been changed.
  3. Verify that the CenterID field has been added (step 2).

@cmadjar
Copy link
Collaborator

cmadjar commented Aug 12, 2021

@kongtiaowang @ridz1208 @driusan we can't figure out why the tests are failing here, could you help @vietdhoang resolve the test issues? Thanks!

@ridz1208
Copy link
Collaborator

@cmadjar it seems like the mri violations module doesnt load at all. If it's not the case, @kongtiaowang could you debug the test ?

@cmadjar
Copy link
Collaborator

cmadjar commented Sep 9, 2021

@driusan @kongtiaowang the test failure does not seem related to the changes in the PR. Could you help us figure out what is going on? Note, the PR has been rebased today.

@cmadjar cmadjar added Critical to release PR or issue is key for the release to which it has been assigned Blocking PR should be prioritized because it is blocking the progress of another task Passed Manual Tests PR has undergone proper testing by at least one peer RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset SQL PR contains SQL modifications such as schema changes and new SQL scripts labels Sep 10, 2021
vietdhoang and others added 4 commits September 24, 2021 14:29
…D. CenterID is a foreign key from the psc table and will be used to indentify the center that the MRI protocol belongs to. If the value is NULL, then the mri_protocol is valid everywhere.
…D field in mri_protocol and for the removal of Center_name
@cmadjar
Copy link
Collaborator

cmadjar commented Sep 24, 2021

HALLELUIA!!! This is passing the tests! Quick, @driusan merge before it does not pass the tests anymore, LOL.

Jokes aside, this one is now in your court. Thank you :).

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@cmadjar
Copy link
Collaborator

cmadjar commented Sep 27, 2021

@driusan I restarted the jobs. The only thing I changed in the last commit was the CHANGELOG so it does not make sense that the tests are suddenly failing since it passed the commit before...

@cmadjar
Copy link
Collaborator

cmadjar commented Sep 27, 2021

@driusan passing the tests. Phew! Ready for you.

@driusan driusan merged commit a0e1cfa into aces:main Oct 6, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking PR should be prioritized because it is blocking the progress of another task Critical to release PR or issue is key for the release to which it has been assigned Passed Manual Tests PR has undergone proper testing by at least one peer RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset SQL PR contains SQL modifications such as schema changes and new SQL scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants