-
Notifications
You must be signed in to change notification settings - Fork 0
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
Move code from jpa to key branch of new main #7
base: main
Are you sure you want to change the base?
Conversation
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.
- the scala tests are currently failing, so we should fix those before merge
- this doesn't look like it will work with patient matching query
@@ -268,7 +269,8 @@ public IPatient retrievePatientById(Integer id) { | |||
|
|||
RawSql rawSql = RawSqlBuilder | |||
.parse(sql) | |||
.columnMapping("id", "patient.id") | |||
.columnMapping("id", "patient_id") |
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.
The column mappings for id and kit id do not work with the new key, you need to use patient.patientKey...
savedSafely = true; | ||
} catch (io.ebean.DuplicateKeyException ex) { | ||
// id must have just been taken, try again | ||
System.out.println("duplicate key exception, try to get a valid id again"); |
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.
If this is an error message you should use log not println
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.
It's not totally an error as this case is just when two devices running on the same try to add a patient at the same time. This is basically a bottle neck to make sure each time a patient is added, each patient gets assigned a unique patient id (for the kit its on). I can change it to a log though, since that makes sense whether or not this is an error. Do we have a logger though?
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.
yes there is a logger
Optional<Patient> maxIdPatient; | ||
Boolean savedSafely = false; | ||
while (!(savedSafely)) { | ||
try { |
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.
this is a lot of logic for one function, maybe we should split this up so the queries especially are in their own functions
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.
like line 356 to 373 should be pulled out
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.
There is only one query here: lines 360 to 364 to get back the largest patient id for that kit. So I could move the query to its own new supportive function, but lines 356 to 373 is basically the whole function.
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.
right, it should be pulled out into one or maybe two
return id; | ||
return patientKey.getPatientId(); | ||
} | ||
|
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.
setters and getters for kit id are missing, if you include them for id then you should include them for kit id too
@@ -7,7 +7,11 @@ | |||
@Sql | |||
public class RankedPatientMatch implements IRankedPatientMatch { | |||
|
|||
@OneToOne | |||
@OneToOne //(fetch = FetchType.EAGER) |
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.
remove these comments
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.
Ya this is a work in progress. If you look at patientEncounter, one of its member variables is patients. I was hoping what worked in patientEncounter would also work with RankedPatientMatch class.
/** | ||
* Getter for patient id. | ||
* | ||
* @return int patient id is the number patients are assigned for identification |
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.
this comment is misleading, patients are not uniquely identifiable anymore just by id
`first_name` VARCHAR(255) NOT NULL , | ||
`last_name` VARCHAR(255) NOT NULL , | ||
`age` INT NOT NULL , | ||
`sex` VARCHAR(10) NOT NULL , | ||
`address` VARCHAR(255) NOT NULL , | ||
`city` VARCHAR(255) NOT NULL , | ||
PRIMARY KEY (`id`) , | ||
PRIMARY KEY (`id`, `kit_id`) , | ||
UNIQUE INDEX `id_UNIQUE` (`id` ASC)) | ||
DEFAULT CHARACTER SET = utf8; |
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.
dont we usually want to create a new evolutions file for db changes? if we were to push this the existing kits would have to rebuild starting at this evolution instead of only adding the new one
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.
Ya I believe you are right. These changes happened before we understood evolutions. I can go back and make new sql files.
@@ -48,10 +49,11 @@ public IPatientEncounter createPatientEncounter(int patientID, DateTime date, in | |||
|
|||
|
|||
try{ | |||
|
|||
// testing |
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.
this comment doesn't provide info about what would need to change for not testing
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.
This was just here because I'm not sure if I should be creating this patientKey here or maybe instead I should pass one in from patient Item or something
} | ||
|
||
//io.ebean.DuplicateKeyException | ||
|
||
return patient; |
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.
general note for this file, there is a get patient by id function. This may now return more than one patient since they are not uniquely identified by id anymore. There should also be a function for get patient by kek
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.
Ya i think you mean get patient by id will need to be changed to get patient by id and kit id?
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.
no, we need get by id for the search bar, so we need to make sure we can handle getting multiple patients back. it would also make sense to have a get by key, but we still need get by id for search bar
this.kitId = kitId; | ||
} | ||
|
||
public PatientKey() {} |
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.
is this needed?
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.
To create a compound key in ebean, an embeddable key is required. This is the class for the embeddable key which represents a patients primary compound key. So I would say this is needed. The other option for a compound key is to do something like @IdClass, but ebean doesn't support that sadly. If it did, that would have been way easier because it wouldn't require a new class and key object.
Program doesn't crash because commented out onetoone columnMapping uses patient.patient_id etc. Add join columns to RankedPatientMatch to reference compound key Use logger Add kit id setter and getters Change patientKey comments Remove un-needed constructor Split savePatient into smaller functions ZZS-70
Add transitionary class CompoundKeyRankedPatientMatch that goes in place of RankedPatientMatch which includes the new kit id to map to. Then these objects are used to create a list of mapped matches which are used to create a list of RankedPatientItems that are displayed in the UI matches. The new class shouldn't be a long term solution and is just to get the feature under test as it just takes all the attributes from rankedpatientmatch and patient and puts them in a class to map the sql database too. ZZS-96
Add kitId as an attribute Add getters and setters ZZS-101
Set patient id back to 0 as the default
Pass composite key to ebean
Sql: set patient id back to asc
Generate patient id in the application
Gets the largest patient id
adds 1 to it and makes that the new patient's id
this patient can now be passed around and inserted into the table
ZZS-70