My modifications to Labrador

[darogan]

There are some ugly code changes such as hard coding the samtools location (I use modules so was the simplest option at the time). And the url for the bam files is also hard linked to launch igv_webstart

Most of my updates have been to coax Labrador into being a generic NGS data viewer as a light-weight portal to sharing data with in my bioinformatics core facility.

[ewels]

Many thanks for this @darogan - I'll try to take a look soon.

It looks like the vast majority of the changes are due to the inclusion of the PHPExcel library. Is it the same as this? If so, could you remove these files and instead just use a git submodule please? Means I can actually read your PR diff, plus keeps the repository less bloated with library code :)

Cheers,

Phil

[ewels]

Thanks @darogan! No need to close the PR..?

[darogan]

OK, thought a new attempt was better. I made some other changes as the Excel reading was not working as it should.

[ewels]

Making a new PR makes sense if they come from different branches or something (so contain different commits). But #16 and this PR both come from master, so contain identical changes.

[darogan]

Had to close the new one and re-open this one Sorry!

[ewels]

No problem! ;)

[darogan]

Example of an inline markdown rendered file

Example of an excel rendered as a very simple html table

[ewels]

Great! Looks like they're currently being formatted as code (eg. monospace font etc) - is that intentional?

I'm just reviewing the PR code now - quite a bit easier now that you converted the packages into submodules :)

[darogan]

I used <pre> for both as I though the monospace might be useful. However I think it can be removed from the markdown.

[ewels]

Hah, you confused my e-mail program:

Makes sense! But yeah, easy enough to do monospace in markdown I guess...

[darogan]

It took 3 attempts to get <pre> to display!

[ewels]

Many thanks for all of your work on this - I've done a code review above, once those things are resolved I'll do a second review where I actually get the site running and check that everything works / looks sensible in my hands. Hope this is ok :)

Phil

[darogan]

Sounds like a good plan.

There are some ugly hacks in a couple of my changes with hard links. Hopefully you will be able to suggest a better solution. My PHP skills are pretty much non existent (although I quite enjoyed hacking Labrador).

[ewels]

Yup - see my comment above. Basically - cut and paste all of the hardcoded stuff into the labrador_config.php file, set as variables and then reference those in the code.

For example, I've done this with stuff like the support e-mail address here, which is defined in the config file here.

[darogan]

@ewels I fixed most of the hard links (I'm sure you will hate my variable names!)

To do: A solution for the samtools location when using linux modules

[ewels]

I think the best samtools solution is just to do the same thing you've done with the other site-specific variables. Maybe add a line above that checks if it's defined and sets it as an empty string if not..

if(!isset($samtools_base)){
  $samtools_base = '';
}
$bam_header = shell_exec (escapeshellcmd ("${samtools_base}samtools view -H $path"));

(or something)

[ewels]

Ok, great! I think that's all I could see in the code. I'll have a stab at running this next week if that's ok, then hopefully merge..

[darogan]

I just removed CHANGES.md. I can add some changes to the README.md instead... probably not today though

[darogan]

A minor update to make the checking of the remaining space on the server more generic, and only applied if set in the config

[darogan]

I removed most of my extra variables in the conf file, and hopefully tidied my use of the url and data variables. I still needed one $data_alias for my apache alias to the data as this is how I'm running multiple labradors at the moment.

[ewels]

Brilliant! Code changes read really nicely now.. I'll start testing the usage now 😁

[ewels]

Ok, it runs fine! I can't test much as although I stole a copy of the database from Babraham a couple of years ago, I don't have any data. But everything I can see from my test copy seems ok.

A few minor things remain:

• There are softlinks to CTR images in img (CTR_600.png, ctr_600_0F7_icon.ico)
  • Shouldn't be needed now that you can reference these from the config?
• Would be good to only show the System status if we have that information:
• Top logo is broken if not set in config:

But after these are resolved, I think we can merge 🎉

[ewels]

Fantastic!

[darogan]

Thanks for adding the merging changes! I hope they will be useful to other folk too.

Will celebrate with :wine_glass:

[ewels]

Absolutely! Thanks for writing them and being so patient with my requests 😉

🍺 for me now.. (and I didn't even do anything!)