Imaging Browser: Make Links to instruments customizable: Redmine 12500

[MounaSafiHarab]

admins can now customize the instruments they want to see under links in the Configution module:

[MounaSafiHarab]

recommendation by @xlecours following his SNP patch Archive/18.0/2017-02-28_SNP_table_structure_check.sql (although his recommendation was based on the knowledge of having to insert one table, instead of 2)

@PapillonMcGill
any better way to conditionally insert?

[PapillonMcGill]

The way to conditionally enter values in Config table if others tables exist is effectively to query INFORMATION_SCHEMA.
On the usage of server side prepared statement, it is usually done to save time on repeated query, not exactly the case here as 2 prepared statements are used. Also, there is no real performance issues here.
In this case, I would use the variable to hold the table names ('mri_parameter_form' or 'radiology_review') and create the prepared statement from the instruction like on https://dev.mysql.com/doc/refman/5.7/en/sql-syntax-prepared-statements.html and execute the statement twice (once with each value).

Although statements are cleared right after, there is a limitation on the number of concurrent prepared statements in a database, so I would limit the usage to only where it is critical to use.

Also, variables are not manually cleared and with last for the duration of the database connection.

Would it be easy to put the SQL code in an 'upgrade' PHP function?

[MounaSafiHarab]

@driusan
you made a comment in the other pull request about projects that do not have those default tables;
but notice the 2 lines below this comment, they do insert without doing any conditional checks on table existence, so I did the same in my additions.

[driusan]

My comment was just in reference to your comment saying that the lines checking validity could removed because there's no way to enter the invalid table names if they don't have them setup, I didn't mean you had to change it.

[MounaSafiHarab]

yes, I know, your point is a valid one. I think the table additions as default should be conditional; but then I realized the 2 lines below my changes are non-conditional either although they use the same tables... not sure how/where they are used and if they cause any issues for projects that don't have the tables.
I will now make the patch for existing projects with the proper conditions as suggested by @PapillonMcGill, then come back to this and see if that same change should be made here

[christinerogers]

@MounaSafiHarab Looks great!! Given this feature is customizable across a single Loris, but not for each entry in the Project table, I have a small clarification suggestion for the PR title and blurb --

• "project customizable" -> "customizable"
• "projects can now customize" --> "admins can now customize"

[PapillonMcGill]

The way to conditionally enter values in Config table if others tables exist is effectively to query INFORMATION_SCHEMA.
On the usage of server side prepared statement, it is usually done to save time on repeated query, not exactly the case here as 2 prepared statements are used. Also, there is no real performance issues here.
In this case, I would use the variable to hold the table names ('mri_parameter_form' or 'radiology_review') and create the prepared statement from the instruction like on https://dev.mysql.com/doc/refman/5.7/en/sql-syntax-prepared-statements.html and execute the statement twice (once with each value).

Although statements are cleared right after, there is a limitation on the number of concurrent prepared statements in a database, so I would limit the usage to only where it is critical to use.

Also, variables are not manually cleared and with last for the duration of the database connection.

Would it be easy to put the SQL code in an 'upgrade' PHP function?

[MounaSafiHarab]

@MounaSafiHarab Looks great!! Given this feature is customizable across a single Loris, but not for each entry in the Project table, I have a small clarification suggestion for the PR title and blurb --

"project customizable" -> "customizable"
"projects can now customize" --> "admins can now customize"

done! I usually use the word "project" in the context of "study", but you are right it is confusing.

[MounaSafiHarab]

@PapillonMcGill

so I spent sometime yesterday and today, I even asked @xlecours for help to try to run this once with the table name as a variable.
None of the examples in the link you provided would work in this case because our situation is a bit more complicated in this case because of the IF clause in @s.
Having said the above, how detrimental it is to have both @s1 and @s2 in the upgrade patch (the way I have it now), given this is run only once at project upgrades?

[PapillonMcGill]

Have you try:
PREPARE stmt FROM (SELECT IF(
(SELECT COUNT(*)
FROM INFORMATION_SCHEMA.TABLES
WHERE table_name = ?
AND table_schema = DATABASE()
) > 0,
"INSERT INTO Config (ConfigID, Value) SELECT cs.ID, 'mri_parameter_form' FROM ConfigSettings cs WHERE cs.Name='ImagingBrowserLinkedInstruments'",
"SELECT 'No' as ?"
));

set @Table1 = "mri_parameter_form";
set @table2 = "radiology_review";
EXECUTE stmt using @Table1, @Table1;
EXECUTE stmt using @table2, @table2;
DEALLOCATE PREPARE stmt;

Also, there is a "?" in your initial request, and it is a place holder for var replacement. Have you tried to escape it?

If nothing work, it is not that bad as it will only be used once. Would have been nice to have clean code but spending hours to solve this is not a good solution either. You could just modify the code to not require variable.

[MounaSafiHarab]

Have you try:
PREPARE stmt FROM (SELECT IF(
(SELECT COUNT(*)
FROM INFORMATION_SCHEMA.TABLES
WHERE table_name = ?
AND table_schema = DATABASE()
) > 0,
"INSERT INTO Config (ConfigID, Value) SELECT cs.ID, 'mri_parameter_form' FROM ConfigSettings cs WHERE cs.Name='ImagingBrowserLinkedInstruments'",
"SELECT 'No' as ?"
));

set @Table1 = "mri_parameter_form";
set @table2 = "radiology_review";
EXECUTE stmt using @Table1, @Table1;
EXECUTE stmt using @table2, @table2;
DEALLOCATE PREPARE stmt;

Also, there is a "?" in your initial request, and it is a place holder for var replacement. Have you tried to escape it?

If nothing work, it is not that bad as it will only be used once. Would have been nice to have clean code but spending hours to solve this is not a good solution either. You could just modify the code to not require variable.

Yes, tried all what you suggested because the link was suggesting using ? which I did initially...
but I just added a commit now based on help from @xlecours to make it work; basically we need @s1 and @s2...
is this better? I find it harder to read but if it is the proper way to do it, then be it

[MounaSafiHarab]

@kongtiaowang
removed the passed manual test as I added a commit

[PapillonMcGill]

Ok, It's fine for this time. As it is single usage, it won't (shouldn't) cause problem in the future.

[MounaSafiHarab]

Ok, It's fine for this time. As it is single usage, it won't (shouldn't) cause problem in the future.

done @PapillonMcGill