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

Extend database and domain for 'analysis performed' and 'comment' #311

Merged
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public class SamplePreview {
private String bioReplicateLabel;
@Column(name = "label")
private String sampleLabel;
private String comment;
@Column(name = "analysis_type")
private String analysisType;
@OneToOne(cascade = CascadeType.ALL)
@JoinColumn(name = "experimentalGroupId")
private ExperimentalGroup experimentalGroup;
Expand All @@ -58,7 +61,7 @@ protected SamplePreview() {
private SamplePreview(ExperimentId experimentId, SampleId sampleId, String sampleCode,
String batchLabel, String bioReplicateLabel,
String sampleLabel, ExperimentalGroup experimentalGroup, String species, String specimen,
String analyte) {
String analyte, String analysisType, String comment) {
Objects.requireNonNull(experimentId);
Objects.requireNonNull(sampleId);
Objects.requireNonNull(sampleCode);
Expand All @@ -79,6 +82,9 @@ private SamplePreview(ExperimentId experimentId, SampleId sampleId, String sampl
this.species = species;
this.specimen = specimen;
this.analyte = analyte;
// optional columns
this.comment = comment;
this.analysisType = analysisType;
}

/**
Expand All @@ -100,15 +106,17 @@ private SamplePreview(ExperimentId experimentId, SampleId sampleId, String sampl
* preview
* @param analyte the {@link Analyte} for the {@link Sample} associated with this
* preview
* @param analysisType the type of analysis to be performed for this {@link Sample}
* @param comment an optional comment pertaining to the associated {@link Sample}
* @return the sample preview
*/
public static SamplePreview create(ExperimentId experimentId, SampleId sampleId,
String sampleCode,
String batchLabel, String bioReplicateLabel,
String sampleLabel, ExperimentalGroup experimentalGroup, String species, String specimen,
String analyte) {
String analyte, String analysisType, String comment) {
return new SamplePreview(experimentId, sampleId, sampleCode, batchLabel, bioReplicateLabel,
sampleLabel, experimentalGroup, species, specimen, analyte);
sampleLabel, experimentalGroup, species, specimen, analyte, analysisType, comment);
}

public ExperimentId experimentId() {
Expand Down Expand Up @@ -146,6 +154,12 @@ public String specimen() {
public String analyte() {
return analyte;
}
public String analysisType() {
return analysisType;
}
public String comment() {
return comment;
}
wow-such-code marked this conversation as resolved.
Show resolved Hide resolved

public ExperimentalGroup experimentalGroup() {
return experimentalGroup;
Expand All @@ -167,14 +181,15 @@ public boolean equals(Object o) {
that.sampleLabel)
&& Objects.equals(species, that.species) && Objects.equals(specimen,
that.specimen) && Objects.equals(analyte, that.analyte) && Objects.equals(
experimentalGroup, that.experimentalGroup);
experimentalGroup, that.experimentalGroup) && Objects.equals(analysisType,
that.analysisType) && Objects.equals(comment, that.comment);
}

@Override
public int hashCode() {
return Objects.hash(experimentId, sampleCode, sampleId, batchLabel, bioReplicateLabel,
sampleLabel,
species, specimen, analyte, experimentalGroup);
species, specimen, analyte, experimentalGroup, analysisType, comment);
}

@Override
Expand All @@ -189,6 +204,8 @@ public String toString() {
", species='" + species + '\'' +
", specimen='" + specimen + '\'' +
", analyte='" + analyte + '\'' +
", analysisType='" + analysisType + '\'' +
", comment='" + comment + '\'' +
", conditions=" + experimentalGroup +
'}';
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import jakarta.persistence.EmbeddedId;
import jakarta.persistence.Entity;
import java.util.Objects;
import java.util.Optional;
import life.qbic.projectmanagement.domain.project.experiment.BiologicalReplicateId;
import life.qbic.projectmanagement.domain.project.experiment.ExperimentId;

Expand Down Expand Up @@ -37,14 +38,17 @@ public class Sample {
@AttributeOverride(name = "uuid", column = @Column(name = "sample_id"))
private SampleId id;
private String label;
private String comment;
@Column(name = "analysis_type")
private String analysisType;
@Embedded
private SampleCode sampleCode;
@Embedded
private SampleOrigin sampleOrigin;

private Sample(SampleId id, SampleCode sampleCode, BatchId assignedBatch, String label,
ExperimentId experimentId, Long experimentalGroupId, SampleOrigin sampleOrigin,
BiologicalReplicateId replicateReference
BiologicalReplicateId replicateReference, Optional<String> analysisType, Optional<String> comment
) {
this.id = id;
this.sampleCode = Objects.requireNonNull(sampleCode);
Expand All @@ -54,6 +58,8 @@ private Sample(SampleId id, SampleCode sampleCode, BatchId assignedBatch, String
this.sampleOrigin = sampleOrigin;
this.biologicalReplicateId = replicateReference;
this.assignedBatch = assignedBatch;
this.analysisType = analysisType.orElse("");
this.comment = comment.orElse("");
Copy link
Member

Choose a reason for hiding this comment

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

I think the sample itself should have null values in these fields as they are optional for the sample.

Copy link
Member Author

Choose a reason for hiding this comment

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

How does this fit with using Optional?

Copy link
Member

Choose a reason for hiding this comment

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

the sample does have nullable fields and optional getters. the sample registration request can have null values. that is fine by me.
In the initial review, I missed the record for the constructor.

}

protected Sample() {
Expand All @@ -75,7 +81,8 @@ public static Sample create(
sampleRegistrationRequest.assignedBatch(),
sampleRegistrationRequest.label(), sampleRegistrationRequest.experimentId(),
sampleRegistrationRequest.experimentalGroupId(),
sampleRegistrationRequest.sampleOrigin(), sampleRegistrationRequest.replicateReference());
sampleRegistrationRequest.sampleOrigin(), sampleRegistrationRequest.replicateReference(),
sampleRegistrationRequest.getAnalysisType(), sampleRegistrationRequest.getComment());
}

public BatchId assignedBatch() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package life.qbic.projectmanagement.domain.project.sample;

import java.util.Objects;
import java.util.Optional;
import life.qbic.projectmanagement.domain.project.experiment.BiologicalReplicateId;
import life.qbic.projectmanagement.domain.project.experiment.ExperimentId;

Expand All @@ -15,21 +16,32 @@
* @param experimentalGroupId the experimental group id the sample is part of
* @param replicateReference the biological replicated reference the sample has been taken from
* @param sampleOrigin information about the sample origin.
* @param analysisType analysis to be performed
* @param comment comment relating to the sample
* @since 1.0.0
*/
public record SampleRegistrationRequest(String label, BatchId assignedBatch,
ExperimentId experimentId, Long experimentalGroupId,
BiologicalReplicateId replicateReference,
SampleOrigin sampleOrigin) {
SampleOrigin sampleOrigin, String analysisType, String comment) {

public SampleRegistrationRequest(String label, BatchId assignedBatch, ExperimentId experimentId,
Long experimentalGroupId, BiologicalReplicateId replicateReference,
SampleOrigin sampleOrigin) {
SampleOrigin sampleOrigin, String analysisType, String comment) {
this.label = Objects.requireNonNull(label);
this.assignedBatch = Objects.requireNonNull(assignedBatch);
this.experimentId = Objects.requireNonNull(experimentId);
this.experimentalGroupId = Objects.requireNonNull(experimentalGroupId);
this.replicateReference = Objects.requireNonNull(replicateReference);
this.sampleOrigin = Objects.requireNonNull(sampleOrigin);
this.comment = comment;
this.analysisType = analysisType;
}

public Optional<String> getAnalysisType() {
return Optional.ofNullable(analysisType);
}
public Optional<String> getComment() {
return Optional.ofNullable(comment);
Copy link
Member

Choose a reason for hiding this comment

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

I read up on it. As soon as you have optional fields you should not use a record. The getter will still give back null so if you call sampleRegistrationRequest.analysisType() you would get null.
We should convert this to a class instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had this problem when I used the original method names "analysisType()" and "comment()". The suggestion to fix this problem was to explicitly add the "get" to the method name. This difference is also displayed by the IDE:
image
See first reply here for the solution I used: https://stackoverflow.com/questions/62945049/java-records-with-nullable-components

Copy link
Member

Choose a reason for hiding this comment

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

yes that happens because there would be two methods with the same signature and name but different return type
public String analysisType() and public Optional<String> analysisType() which is not possible in Java. That is why I recommend overwriting the method and making it a class instead of a record or not using optional in this case but simply returning null and make use of optional in the Sample class.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class SampleRegistrationServiceSpec extends Specification {
def "Invalid SampleRegistrationRequests returns a Result containing a SAMPLE_REGISTRATION_FAILED response code"() {
given:
SampleOrigin sampleOrigin = SampleOrigin.create(new Species("species"), new Specimen("specimen"), new Analyte("analyte"))
SampleRegistrationRequest sampleRegistrationRequest = new SampleRegistrationRequest("my_label", BatchId.create(), ExperimentId.create(), 5, BiologicalReplicateId.create(), sampleOrigin)
SampleRegistrationRequest sampleRegistrationRequest = new SampleRegistrationRequest("my_label", BatchId.create(), ExperimentId.create(), 5, BiologicalReplicateId.create(), sampleOrigin, "mytype", "no comment")
SampleCode sampleCode = SampleCode.create("QABCDE")
sampleCodeService.generateFor(projectId) >> Result.fromValue(sampleCode)
Map<SampleCode, SampleRegistrationRequest> sampleCodesToRegistrationRequests = new HashMap<>()
Expand All @@ -53,7 +53,7 @@ class SampleRegistrationServiceSpec extends Specification {
def "Valid SampleRegistrationRequests returns a Result with the list of registered Samples"() {
given:
SampleOrigin sampleOrigin = SampleOrigin.create(new Species("species"), new Specimen("specimen"), new Analyte("analyte"))
SampleRegistrationRequest sampleRegistrationRequest = new SampleRegistrationRequest("my_label", BatchId.create(), ExperimentId.create(), 4, BiologicalReplicateId.create(), sampleOrigin)
SampleRegistrationRequest sampleRegistrationRequest = new SampleRegistrationRequest("my_label", BatchId.create(), ExperimentId.create(), 4, BiologicalReplicateId.create(), sampleOrigin, "this analysis type", "a comment")
SampleCode sampleCode = SampleCode.create("QABCDE")
Sample sample = Sample.create(sampleCode, sampleRegistrationRequest)
sampleCodeService.generateFor(projectId) >> Result.fromValue(sampleCode)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class SampleDomainServiceSpec extends Specification {

def "When a sample has been successfully registered, a sample registered event is dispatched"() {
given:
Sample testSample = Sample.create(SampleCode.create("test"), new SampleRegistrationRequest("test sample", BatchId.create(), ExperimentId.create(), 1L, BiologicalReplicateId.create(), new SampleOrigin(new Species("test"), new Specimen("test"), new Analyte("test"))))
Sample testSample = Sample.create(SampleCode.create("test"), new SampleRegistrationRequest("test sample", BatchId.create(), ExperimentId.create(), 1L, BiologicalReplicateId.create(), new SampleOrigin(new Species("test"), new Specimen("test"), new Analyte("test")), "DNA analysis", ""))

and:
SampleRepository testRepo = Mock(SampleRepository)
Expand Down Expand Up @@ -54,7 +54,7 @@ class SampleDomainServiceSpec extends Specification {

when:
Result<Sample, SampleDomainService.ResponseCode> result = sampleDomainService.registerSample(SampleCode.create("test"),
new SampleRegistrationRequest("test sample", BatchId.create(), ExperimentId.create(), 1L, BiologicalReplicateId.create(), new SampleOrigin(new Species("test"), new Specimen("test"), new Analyte("test"))))
new SampleRegistrationRequest("test sample", BatchId.create(), ExperimentId.create(), 1L, BiologicalReplicateId.create(), new SampleOrigin(new Species("test"), new Specimen("test"), new Analyte("test")), "DNA analysis", ""))

then:
sampleRegistered.batchIdOfEvent.equals(result.getValue().assignedBatch())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
* Sample Overview Component
* <p>
* Component embedded within the {@link SampleInformationPage} in the {@link ProjectViewPage}. It
* allows the user to see the information associated for all {@link Batch} and {@link Sample} of
* allows the user to see the information associated for each {@link Batch} and {@link Sample} of
* each
* {@link Experiment within a {@link life.qbic.projectmanagement.domain.project.Project}
* Additionally it enables the user to register new {@link Batch} and {@link Sample} via the
Expand Down Expand Up @@ -289,6 +289,8 @@ private Grid<SamplePreview> createSampleGrid() {
sampleGrid.addColumn(SamplePreview::species).setHeader("Species");
sampleGrid.addColumn(SamplePreview::specimen).setHeader("Specimen");
sampleGrid.addColumn(SamplePreview::analyte).setHeader("Analyte");
sampleGrid.addColumn(SamplePreview::analysisType).setHeader("Analysis to Perform");
sampleGrid.addColumn(SamplePreview::comment).setHeader("Comment");
return sampleGrid;
}

Expand Down Expand Up @@ -377,7 +379,8 @@ private List<SampleRegistrationRequest> createSampleRegistrationRequests(BatchId
return new SampleRegistrationRequest(sampleRegistrationContent.label(), batchId,
experimentId,
sampleRegistrationContent.experimentalGroupId(),
sampleRegistrationContent.biologicalReplicateId(), sampleOrigin);
sampleRegistrationContent.biologicalReplicateId(), sampleOrigin,
sampleRegistrationContent.analysisType(), sampleRegistrationContent.comment());
}).toList();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@
* @param species String representation of the {@link Species}
* @param specimen String representation of the {@link Specimen}
* @param analyte String representation of the {@link Analyte}
* @param analysisType The analysis to be performed
* @param comment Sample specific comments
*/

public record SampleRegistrationContent(String label, BiologicalReplicateId biologicalReplicateId,
Long experimentalGroupId,
String species, String specimen, String analyte,
String species, String specimen, String analyte, String analysisType,
String comment) {

}
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ private List<SampleRegistrationContent> getContent() {
SampleRegistrationContent sampleRegistrationContent = new SampleRegistrationContent(
row.sampleLabel(), row.bioReplicateID(), row.experimentalGroupId(), row.species(),
row.specimen(),
row.analyte(), row.customerComment());
row.analyte(), row.analysisType(), row.customerComment());
samplesToRegister.add(sampleRegistrationContent);
});
return samplesToRegister;
Expand Down