Skip to content

[imaging Browser] Fix wrong data being displayed under QC columns#6654

Merged
driusan merged 2 commits into
aces:23.0-releasefrom
h-karim:2020-6-1-ImagingBrowserFixHumanError
Jun 5, 2020
Merged

[imaging Browser] Fix wrong data being displayed under QC columns#6654
driusan merged 2 commits into
aces:23.0-releasefrom
h-karim:2020-6-1-ImagingBrowserFixHumanError

Conversation

@h-karim

@h-karim h-karim commented Jun 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Wrong data ("Human") is currently appearing under the QC columns of some scan types. This is due to a bug in how the modalities_subquery values are being assigned.

Brief summary of changes

  • I changed the modalities subquery reassignment under the for loop to a concatenation.

Note: for the file changes, the linter automatically removed some whitespace at the end of some lines after saving.

Testing instructions (if applicable)

  1. Having 2+ scan type columns (you can add more through admin>configuration>imaging modules>tabulated scan types, go to the front page
  2. Observe no "Human" values under the QC statuses of the different scan types.

Link(s) to related issue(s)

christinerogers
christinerogers requested changes Jun 1, 2020
View reviewed changes

@christinerogers christinerogers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM -- two small changes please:

  • see the space i suggested in the modalities_subquery
  • Add 1 short sentence to the PR header to describe what this fixes in the module, and why it was happening.
    pls request my re-review after. thanks

Comment thread modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc Outdated
@h-karim h-karim requested a review from christinerogers June 2, 2020 13:24
@christinerogers

christinerogers commented Jun 2, 2020

Copy link
Copy Markdown
Contributor

@h-karim did you address both my review points? Not sure I'm seeing the second one in action.

@cmadjar cmadjar mentioned this pull request Jun 2, 2020
Closed
@cmadjar cmadjar linked an issue Jun 2, 2020 that may be closed by this pull request
[imaging browser] Adding some scan types renders the module unloadable #6616
Closed
@christinerogers christinerogers added 23.0.0-testing Category: Bug PR or issue that aims to report or fix a bug labels Jun 2, 2020
@h-karim h-karim changed the title [imaging Browser] Fix concatenation mistake for modalities_subquery [imaging Browser] Fix wrong data being displayed under QC columns Jun 2, 2020
@driusan driusan added the Critical to release PR or issue is key for the release to which it has been assigned label Jun 2, 2020
@laemtl laemtl assigned h-karim and unassigned laemtl Jun 2, 2020
@laemtl laemtl self-requested a review June 2, 2020 23:14
@laemtl laemtl added the Passed manual tests PR has been successfully tested by at least one peer label Jun 3, 2020
laemtl
laemtl approved these changes Jun 3, 2020
View reviewed changes
Comment thread modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc Outdated
@laemtl laemtl self-requested a review June 3, 2020 20:25
laemtl
laemtl requested changes Jun 3, 2020
View reviewed changes

@laemtl laemtl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment above!

@h-karim h-karim requested a review from laemtl June 3, 2020 20:53
driusan
driusan reviewed Jun 4, 2020
View reviewed changes
Comment thread modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc Outdated
@christinerogers christinerogers requested review from christinerogers and removed request for christinerogers June 4, 2020 17:43
@christinerogers christinerogers self-requested a review June 5, 2020 14:12
@h-karim h-karim requested a review from driusan June 5, 2020 14:56
@christinerogers christinerogers requested a review from laemtl June 5, 2020 15:29
@christinerogers christinerogers removed the Passed manual tests PR has been successfully tested by at least one peer label Jun 5, 2020
@christinerogers

christinerogers commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

@laemtl could you pull and test this today? it was recently updated.
If it passes, please add the Passed Manual Test label back on. thanks

@driusan

driusan commented Jun 5, 2020

Copy link
Copy Markdown
Collaborator

This both needs to be rebased (has conflicts and can't be merged) and have Travis run on it.

@laemtl laemtl added the Passed manual tests PR has been successfully tested by at least one peer label Jun 5, 2020
@christinerogers christinerogers added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 5, 2020
@h-karim h-karim requested a review from laemtl June 5, 2020 16:18
@h-karim

h-karim commented Jun 5, 2020

Copy link
Copy Markdown
Contributor Author

Performed a rebase, @laemtl please test this again

@h-karim h-karim removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) Passed manual tests PR has been successfully tested by at least one peer labels Jun 5, 2020
@laemtl laemtl added the Passed manual tests PR has been successfully tested by at least one peer label Jun 5, 2020
@christinerogers

christinerogers commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

@cmadjar @nbro would you like to test?
It's ready for final review by @driusan

christinerogers
christinerogers approved these changes Jun 5, 2020
View reviewed changes

@christinerogers christinerogers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@driusan driusan closed this Jun 5, 2020
@driusan driusan reopened this Jun 5, 2020
driusan
driusan approved these changes Jun 5, 2020
View reviewed changes

@driusan driusan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If Travis approves it looks fine to me..

@driusan

driusan commented Jun 5, 2020

Copy link
Copy Markdown
Collaborator

This passed Travis, the icon just didn't update..

@driusan driusan merged commit f301c86 into aces:23.0-release Jun 5, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@driusan driusan driusan approved these changes
@christinerogers christinerogers christinerogers approved these changes
@laemtl laemtl laemtl approved these changes
@AlexandraLivadas AlexandraLivadas Awaiting requested review from AlexandraLivadas

Assignees

@h-karim h-karim

Labels

Category: Bug PR or issue that aims to report or fix a bug Critical to release PR or issue is key for the release to which it has been assigned Passed manual tests PR has been successfully tested by at least one peer

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

[Imaging browser] 'Human' value being displayed under QC status [imaging browser] Adding some scan types renders the module unloadable

5 participants

@h-karim @christinerogers @driusan @laemtl @ridz1208