-
-
Notifications
You must be signed in to change notification settings - Fork 727
Update CancerStudy with imaging sample counts #11463
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
base: master
Are you sure you want to change the base?
Conversation
|
eb83e6e
to
9b3f196
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gblaih Thanks for making efforts and making this aviablable! This is great!
Just one quick comment: I think we can go ahead and merge this as a solid initial approach. That said, we should also explore ways to automatically discover new resource types. For example, if we add a new imaging type like new_image_type, it would be ideal for it to appear on the homepage automatically. Even if this requires introducing a new endpoint or doing some additional refactoring, I believe it's worth the effort.
a31ff3e
to
1f45edc
Compare
ddbf315
to
0800a6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gblaih Overall, the PR looks great and works well for demo purposes. My comments are mainly focused on improving code clarity and aligning better with API design principles. Here's a breakdown:
-
Rename
resources
toresourceCounts
To better reflect the data type (List<ResourceCount>
), let's rename related variables and methods in the model, service, and repository layers. (Apologies for initially suggestingresources
— your originalResourceCount
naming is actually more descriptive.) -
Conditionally include
resourceCounts
based onprojection
Sinceprojection
controls the response detail in our API, we should includeresourceCounts
only whenprojection
isSUMMARY
orDETAILED
. We can handle this with a simple conditional in the service layer. -
Add support in
fetchStudies
fetchStudies
is the third method that needs to addresourceCounts
, but it’s currently missing from the implementation. -
Remove hard‑coded
CHROMOSCOPE
andFIGURES
Let’s avoid hardcoding these in the backend. Instead, handle them in the frontend—either by hardcoding there or by leveraging the new JSON field.
src/main/java/org/cbioportal/legacy/service/impl/StudyServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cbioportal/legacy/service/impl/StudyServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/resources/org/cbioportal/legacy/persistence/mybatis/StudyMapper.xml
Outdated
Show resolved
Hide resolved
src/main/java/org/cbioportal/legacy/service/impl/StudyServiceImpl.java
Outdated
Show resolved
Hide resolved
e5fdad7
to
32368e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your updates looks great! I just have two more comments related to the mapper file
src/main/resources/org/cbioportal/legacy/persistence/mybatis/StudyMapper.xml
Outdated
Show resolved
Hide resolved
src/main/resources/org/cbioportal/legacy/persistence/mybatis/StudyMapper.xml
Outdated
Show resolved
Hide resolved
32368e5
to
161b1d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me! Thanks!
161b1d7
to
94874b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gblaih The tests are indicating an real issue, I have some explainations in comments
<where> | ||
<if test="studyIds != null and studyIds.size > 0"> | ||
<if test="studyIds != null and !studyIds.isEmpty()"> | ||
cancer_study.CANCER_STUDY_IDENTIFIER IN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the issue:
need to replace this with
cs.CANCER_STUDY_IDENTIFIER IN
The reason is when you give a table an alias (e.g. cancer_study cs
), that alias cs
replaces the original table name in the rest of the query. Using the full table name afterward will cause an error
<where> | ||
<if test="studyIds != null and studyIds.size > 0"> | ||
<if test="studyIds != null and !studyIds.isEmpty()"> | ||
cancer_study.CANCER_STUDY_IDENTIFIER IN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
|
654ebf2
to
4b06f4f
Compare
4b06f4f
to
be840ee
Compare
Update CancerStudy type with field
resourceCounts
that holds resource information and counts within study and dynamically calculate these resource counts