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

[DSM] PEPPER-853 sm id fix #2636

Merged
merged 7 commits into from
Aug 4, 2023
Merged

[DSM] PEPPER-853 sm id fix #2636

merged 7 commits into from
Aug 4, 2023

Conversation

pegahtah
Copy link
Contributor

Context

I added a check for zero received sm ids.
I trired using varioable assignment but it doesn't work in java

Checklist

  • I have labeled the type of changes involved using the C-* labels.
  • I have assessed potential risks and labeled using the R-* labels.
  • I have considered error handling and alerts, and added L-* labels as needed.
  • I have considered security and privacy, and added I-* labels as needed
  • I have analyzed my changes for stability, fault tolerance, graceful degradation, performance bottlenecks and written a brief summary in this PR.
  • If applicable, I have discussed the analytics needs at both a platform and study level with Product and instrumented code accordingly.
  • If applicable, my UI/UX changes have passed muster with Product/Design via an over-the-shoulder review, screenshots, etc.

If unsure or need help with any of the above items, add the help wanted label. For items that starts with If applicable, if it is not applicable, check it off and add n/a in front.

FUD Score

Overall, how are you feeling about these changes?

  • ☺️ All good, business as usual!
  • 😅 There might be some issues here
  • 😱 I'm sweaty and nervous

How do we demo these changes?

How does one observe these changes in a deployed system? Note that user visible encompasses many personas--not just patients and study staff, but also ops duty, your fellow devs, compliance, etc.

  • They are user-visible in dev as a regular user journey and require no additional instructions.
  • Getting dev into a state where this is user-visible requires some tech fiddling. I have documented these steps in the related ticket.
  • Requires other features before it's human visible. I have documented the blocking issues in jira.
  • I have no idea how to demo this. Please help me!

Testing

  • I have written automated positive tests
  • I have written automated negative tests
  • I have written zero automated tests but have poked around locally to verify proper functionality
  • The jira ticket has acceptance criteria and QA has the needed information to test changes

Release

  • These changes require no special release procedures--just code!
  • Releasing these changes requires special handling and I have documented the special procedures in the release plan document

@pegahtah pegahtah added good first issue Good for newcomers C-bug Category: identifies or fixes a bug C-feature Category: non-breaking change which adds new functionality dsm DDP Study Manager labels Jul 28, 2023
Copy link
Contributor

@denniscunningham denniscunningham left a comment

Choose a reason for hiding this comment

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

We should plan to refactor this query (perhaps another ticket?), even if that means executing more than one query, since the logic of this query is lost in the complexity.

For this case (PEPPER-853), is in possible to add logic to check the query results rather than adding more logic (and complexity) to the query?

@pegahtah
Copy link
Contributor Author

pegahtah commented Aug 3, 2023

@denniscunningham I simplified the query to just check if the numbers entered for both USS and scrolls count are simultaneously 0 or NULL and not show those tissues.

@@ -28,6 +28,7 @@ public class MercurySampleDao implements Dao<MercurySampleDto> {
+ "LEFT JOIN ddp_onc_history_detail oD on (mr.medical_record_id = oD.medical_record_id AND NOT oD.deleted <=> 1) "
+ "LEFT JOIN ddp_tissue t on (oD.onc_history_detail_id = t.onc_history_detail_id AND NOT t.deleted <=> 1) "
+ "WHERE oD.tissue_received IS NOT NULL AND p.ddp_participant_id = ? AND ddp.instance_name = ? "
+ "AND (IFNULL(t.uss_count, 0) <> 0 OR IFNULL(t.scrolls_count, 0) <> 0) "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just avoiding the ones that are both 0 or both null should do the trick.

Copy link
Contributor

@denniscunningham denniscunningham left a comment

Choose a reason for hiding this comment

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

Looks good Pegah. Thx for the changes!
Just left one comment.

@@ -40,7 +41,7 @@ public class MercurySampleDao implements Dao<MercurySampleDto> {
+ "From ddp_mercury_sequencing where ddp_participant_id = ? group by (tissue_id) ) as table2 "
+ "on table2.seqt = table1.tissue_id "
+ "LEFT JOIN (select order_id, mercury_sequencing_id, order_status, order_date, mercury_pdo_id "
+ "FROM ddp_mercury_sequencing WHERE ddp_participant_id = ?) as table3 ON(table3.mercury_sequencing_id = table2.ms_id)";
+ "FROM ddp_mercury_sequencing WHERE ddp_participant_id = ?) as table3 ON(table3.mercury_sequencing_id = table2.ms_id) ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I like to have an extra space at the end in case we need to concat the query with some other condition

@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.15) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@pegahtah pegahtah merged commit ac44315 into develop Aug 4, 2023
5 checks passed
@pegahtah pegahtah deleted the PEPPER-853_zeroSmIdFix branch August 4, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: identifies or fixes a bug C-feature Category: non-breaking change which adds new functionality dsm DDP Study Manager good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants