Feature/36213/st georges crc importer new by NImeson · Pull Request #81 · NHSDigital/data_management_system

NImeson · 2024-06-10T13:42:24Z

What?
A new St Georges importer has been written to account for a new data format (see planio ticket 36213).

Why?
A new importer had to be created to be able to parse St Georges colorectal data.

How?
The importer was written based on the rules sent by Fiona.

Testing?
Unittests have been written. A QA of the variant counts has also been completed and signed off by Fiona.

Note: git commit history for the work can be found in branch feature/36213/st_georges_crc_importer, this original branch was created from the 'main' branch in error, instead of the 'develop' branch. Code has been copied across to this new branch forked from 'develop'.

ollietulloch · 2024-06-10T14:50:15Z

I don't think this is needed?

removed` in 075175e

ollietulloch · 2024-06-10T14:56:58Z

I can't see any test coverage for process_fields?

Test added 075175e

ollietulloch · 2024-06-10T15:09:57Z

I don't think you need to reassign genotype here, it's being mutated in the assign_test_scope method.

removed reassignment in 075175e

ollietulloch · 2024-06-10T15:11:54Z

You won't need to return the genotype here if process_fields doesn't reassign genotype.

ollietulloch · 2024-06-10T15:14:17Z

You could assign DateTime.parse('18/07/2022') to a variable, rather than repeating the code snippet on L125.

The logic could be simplified, something like:

date = DateTime.parse(record.raw_fields['authoriseddate']) panel_change_date = DateTime.parse('18/07/2022') genes = %w[APC BMPR1A EPCAM MLH1 MSH2 MSH6 MUTYH NTHL1 PMS2 POLD1 POLE PTEN SMAD4 STK11] date < panel_change_date ? genes : genes + %w[GREM1 RNF43]

simplified logic here in 075175e

ollietulloch · 2024-06-10T15:29:33Z

I think this could be simplified a little, something like:

columns = if genotype.targeted? ['gene', 'gene (other)', 'variant_dna', 'variant protein', 'test/panel'] elsif genotype.full_screen? ['gene', 'gene (other)', 'variant_dna', 'test/panel'] end duplicate_genotype(columns, genotype, genes, record) if columns.present?

Simplified logic in 075175e

ollietulloch · 2024-06-10T16:06:47Z

Could the class name match the file name please? e.g. StGeorgeHandlerColorectalTest

ammended in 075175e

NImeson · 2024-06-10T16:21:59Z

#77 (comment) - Take note of this comment for addition to CRC importer as well

Added logging method in CRC importer - should be ther in 075175e

lauramccluskey1 · 2024-06-10T16:24:19Z

Instead of assigning no genetic test scope, get it to give an error message, so we can flag if this ever happens and set new rules(see comment on code review for #77)

Added logging method in CRC importer - should be there in 075175e

lauramccluskey1 · 2024-06-14T06:30:21Z

+    process_fields_wrecord = build_raw_record('pseudo_id1' => 'bob')
+    process_fields_wrecord.raw_fields['servicereportidentifier'] = 'W1234567'
+    @handler.process_fields(process_fields_wrecord)
+    assert_not_equal 'W1234567', @genotype.attribute_map['servicereportidentifier']


I think this would not be equal to the SRI even it was starting with 'V' , as process fields creates a new genotype

Changes made in d16287a

lauramccluskey1 · 2024-06-14T07:22:07Z

+          def interrogate_variant_protein_fullscreen(record, genotype, column)
+            if record.raw_fields['variant protein'].blank?
+              genotype.add_status(1)
+            elsif record.raw_fields['variant protein'].match(/fail/ix)


These are in a different order to the rules- can you check this?

good spot, changes made in d16287a

lauramccluskey1 · 2024-06-14T07:45:27Z

+
+          def handle_test_status(record, genotype, genes)
+            columns = if genotype.targeted?
+                        ['gene', 'gene (other)', 'variant_dna', 'variant protein', 'test/panel']


I don't think you need all these columns here? Probably a good idea to check these alongside the comment from @shilpigoeldev at line 70

Corrected in latest commit

shilpigoeldev · 2024-06-14T15:38:22Z

@NImeson , I don't think colorectal script to run 'RJ7' has been added yet , can you please commit that as well.

NImeson · 2024-06-17T07:57:57Z

@NImeson , I don't think colorectal script to run 'RJ7' has been added yet , can you please commit that as well.

Which file do you mean @shilpigoeldev ? the entry should be there in the bash script
.../Import_all_colorectal_interactive.sh. Is there another one missing?

@NImeson , Sorry , I missed that earlier..yes its present so nothing needed for this.

shilpigoeldev · 2024-06-20T14:59:01Z

+              gene_list.each do |gene|
+                CRC_GENE_MAP[gene]&.each do |gene_value|
+                  genes.append(gene_value)
+                end


There is no need of CRC_GENE_MAP I suppose as we don't have any mapping being done here . In BRCA it was needed as BR1 -> BRCA1 but nothing needed here , so should get rid of it.

There is a common typo for MSH2 (MHS2) which is mapped back to MSH2, I think this is why I included it

shilpigoeldev · 2024-06-20T15:07:53Z

+            # extracts panels tested from record
+            # map panels to list of genes in FULL_SCREEN_TESTS_MAP
+            # return list of genes tested in panel
+            panel_genes_list = FULL_SCREEN_TESTS_MAP[record.raw_fields['test/panel']]


Can you make this lookup as case insensitive like using downcase on record.raw_fields['test/panel'], as being done for adding scope?

This is mapping the test panels (e.g. R209 etc) - should this be the same for BRCA also then? I think it is not lower case for BRCA

shilpigoeldev

All good !!

ollietulloch reviewed Jun 10, 2024

View reviewed changes

NImeson closed this Jun 13, 2024

NImeson force-pushed the feature/36213/st_georges_crc_importer_new branch from a52f508 to 2e492af Compare June 13, 2024 11:44

re-adding changes after erroneous git rebase

075175e

NImeson reopened this Jun 13, 2024

NImeson added 2 commits June 13, 2024 13:15

rucocop changes added

9008c0c

fixed failing test

a2d25f8

shilpigoeldev requested changes Jun 13, 2024

View reviewed changes

Comment thread lib/import/colorectal/providers/st_george/st_george_handler_colorectal.rb

Comment thread lib/import/colorectal/providers/st_george/st_george_handler_colorectal.rb Outdated

lauramccluskey1 reviewed Jun 14, 2024

View reviewed changes

NImeson added 2 commits June 14, 2024 16:15

made code review changes, removed zygosity

a54a47f

made code review changes, removed zygosity

d16287a

NImeson added 2 commits June 17, 2024 13:51

adding final change from PR and fix teststatus issue

74f2ee7

importer wasn't assigning moleculsr testing type - corrected this

1ba4832

NImeson marked this pull request as draft June 19, 2024 12:01

NImeson marked this pull request as ready for review June 19, 2024 13:39

shilpigoeldev requested changes Jun 20, 2024

View reviewed changes

made PR changes, rubocop fixes, added tests

260bc18

NImeson requested a review from shilpigoeldev June 21, 2024 11:30

making test_type mapping case insensitive

32fe9b9

shilpigoeldev previously approved these changes Jun 21, 2024

View reviewed changes

Merge branch 'develop' into feature/36213/st_georges_crc_importer_new

b4d765a

shilpigoeldev dismissed their stale review via b4d765a June 21, 2024 13:23

NImeson added 2 commits June 21, 2024 14:37

removed barts mapping, added extra line to constants

718a4c5

resolved conflicts, removed barts from import

63c095c

NImeson requested a review from shilpigoeldev June 21, 2024 13:44

Rubocop indentation fixes

8889223

shilpigoeldev approved these changes Jun 21, 2024

View reviewed changes

NImeson merged commit 04d1357 into develop Jun 21, 2024

NImeson deleted the feature/36213/st_georges_crc_importer_new branch June 21, 2024 14:11

Uh oh!

Conversation

NImeson commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NImeson Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ollietulloch Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NImeson commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shilpigoeldev commented Jun 14, 2024

Uh oh!

NImeson commented Jun 17, 2024 • edited by shilpigoeldev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NImeson Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shilpigoeldev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

NImeson Jun 13, 2024 •

edited

Loading

ollietulloch Jun 10, 2024 •

edited

Loading

NImeson commented Jun 10, 2024 •

edited

Loading

NImeson commented Jun 17, 2024 •

edited by shilpigoeldev

Loading

NImeson Jun 21, 2024 •

edited

Loading