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

Imaging Browser: Make Links to instruments project customizable and add the DICOM download: Redmines 12500 and 12580 #3496

Conversation

MounaSafiHarab
Copy link
Contributor

@MounaSafiHarab MounaSafiHarab commented Feb 15, 2018

As the title says: projects can now customize the instruments they want to see under links, and can download the DICOMs from Imaging Browser.

The second feature was started in #2791; but finished here.

The instruments are chosen as shown in screenshot below:
config

and

the Imaging Browser View Session Links would display:
links

…dd the DICOM download: Redmines 12500 and 12580
@MounaSafiHarab MounaSafiHarab added Category: Feature PR or issue that aims to introduce a new feature [branch] minor labels Feb 15, 2018
@MounaSafiHarab MounaSafiHarab added this to the 19.1 milestone Feb 15, 2018
@MounaSafiHarab MounaSafiHarab added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 15, 2018
@MounaSafiHarab
Copy link
Contributor Author

MounaSafiHarab commented Feb 15, 2018

not sure where to put the .sql patch within the Archive directory;
and I need to rebase on minor but that caused the change to be many more files that it should be, so will rebase once minor is in synch with 19.0-dev

= $DB->tableExists('mri_parameter_form');
$this->tpl_data['rad_review_table_exists'] = $DB->tableExists(
'radiology_review'
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tables should exist otherwise the user would not be able to select them in the Configuration module in the first place; so I thought these was no need for these few lines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you include them as defaults in the patch? What's that going to do if they don't have the tables?

Copy link
Contributor Author

@MounaSafiHarab MounaSafiHarab Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was the case for me for radiology_review table which does not exist in my dump; the patch will just simply run and won't add it without any error. But maybe it is best I add conditional addition (if table exists) to the patch?

@MounaSafiHarab
Copy link
Contributor Author

MounaSafiHarab commented Feb 15, 2018

@ridz1208

IF this makes it to Loris, we will have one less override for CCNA (the good news for me, it happens to be the only one I need to maintain!)!

@driusan
Copy link
Collaborator

driusan commented Feb 19, 2018

@MounaSafiHarab This is tagged milestone 19.1 but going to branch 19.0. Can you fix one or the other?

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will review after it's split..

@@ -0,0 +1,6 @@
-- Add Imaging Browser to Imaging Modules
INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'linkedInstruments', 'Instruments that the users want to see linked from Imaging Browser', 1, 1, 'instrument', ID, 'Imaging Browser Links to Instruments', 7 FROM ConfigSettings WHERE Name="imaging_modules";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the name to something link "ImagingBrowserLinkedInstruments"? "linkedInstruments" as a generic config name used throughout LORIS isn't really clear.

$FullPath = $tarchivePath . '/' . $File;
$MimeType = 'application/x-tar';
$DownloadFilename = basename($File);
break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split this into 2 PRs? The DICOM Tar download and the imaging browser linked instruments don't belong together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will. I just wanted to see if there is interest in this. But I guess I will split them, basing it off of minor, and take care of your other comments in the newly sent pull requests to make it easier for review.

@MounaSafiHarab
Copy link
Contributor Author

@MounaSafiHarab This is tagged milestone 19.1 but going to branch 19.0. Can you fix one or the other?

yes @driusan
am aware of it, I had placed a comment earlier about it and added the needs rebase tag for that specifically. I wanted to be based off of 19.0-dev and sending this from the minor branch would have showed way too many changes irrelevant to this pull request. Will minor be updated anytime soon?

@driusan
Copy link
Collaborator

driusan commented Feb 19, 2018

@MounaSafiHarab hopefully!

@MounaSafiHarab
Copy link
Contributor Author

closing, as this pull request is divided into these two:
#3500
and
#3501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants