-
Notifications
You must be signed in to change notification settings - Fork 174
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
Flag_Status field in flag
table appears to be unused
#8278
Conversation
@@ -0,0 +1 @@ | |||
ALTER TABLE `flag` DROP COLUMN `Flag_status`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's dropping a column that might have data and existing projects might not want to run it should go in cleanup patches (which need to be manually run after review) instead of new_patches (which get concatenated and into one big release patch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will drop it under the cleanup patches then!
Should this update raisinbread too? |
@driusan I haven't seen any |
You're dropping a column from the flag table so I assume the raisinbread flag table has data for it? |
@driusan I am not sure, I have checked it and can't see any |
It's in https://github.com/aces/Loris/blob/main/raisinbread/RB_files/RB_flag.sql. I'm not sure what the preferred way to update it is, maybe @ridz1208 knows. |
@ridz1208 What's your opinin for updating the Flag_status on https://raw.githubusercontent.com/aces/Loris/main/raisinbread/RB_files/RB_flag.sql ? |
@GeorgeMurad last I checked there was a tools/exporters/db_data...(something)...php that will dump your current DB into the raisinbread directory. so basically, if you source RB, run your patch on it and run the tool script. you should see that the RB_flag.sql file has changed (sometimes other files change for whitespacing and stuff you can ignore those) but yeah that should do it for you |
@kongtiaowang I think your change looks like it's done in the last commit. Can you review and if it looks good assign to me to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @driusan this PR is ready for your to merge.
@GeorgeMurad This needs to be rebased before I can merge it to make sure the required tests pass. |
Create new patch file under SQL/New_patches to Drop the unused
Flag_status
field underflag
table.Remove the
Flag_status
SQL/0000-00-00-schema.sql file.Remove the
Flag_status
field fromLoris/test/unittests/NDB_BVL_Instrument_Test.php
Line 1150 in fa666f5