contact id fix #5610
contact id fix #5610
Conversation
|
@psoas and @adunsulag , Fixed bug where modifying a already existing address was creating a new contact entry. When add a new address, it is still creating a new contact entry for same patient though (is this meant to happen or are all the contact_address entries for the same patient supposed to link to the same contact entry?). |
|
just added another commit to deal with my above comment/question: Not sure though if we are only allowing one contact per foreign table/id combination, so will need to await input before bringing this in. |
|
@bradymiller @adunsulag |
|
@bradymiller @adunsulag I don't think that putting a query in there is the correct fix. I know there is a rush and stephen is driving cross country. I have to look at the code tonite after I take my kids to the zoo. It would probably take me a couple hours to figure it out. Unless Stephen gets to it before me. |
| $this->_id = $id; | ||
| $this->id = $id; | ||
|
|
There was a problem hiding this comment.
Yep this is reason update was creating a new contact record.
src/Common/ORDataObject/Contact.php
Outdated
| // if entry already exists in table, then set the id | ||
| $id = sqlQuery("SELECT `id` FROM `contact` WHERE `foreign_table_name` = ? AND `foreign_id` = ?", [$foreign_table_name, $foreign_id])['id'] ?? null; | ||
| if (!empty($id)) { | ||
| $this->id = $id; |
There was a problem hiding this comment.
@bradymiller
Since the contact table is just for mapping what tables additional addresses are used and not for mapping each address entry then I don't see a recourse other than to validate if an existing mapping already exists and carry the id forward.
While this fix will work on updating an existing or adding a single new address, it will fail when adding more than one additional address when an existing contact doesn't already exist. Say new patient and adding several additional addresses on data entry.
This is because we build a array of records to save before we persist(I think an eye towards transaction). Since a contact map for the patient doesn't exist, then a new sequence is generated populating the records array(in memory) then persisted.
The contact id should be tested for and set in a property in contact service construct somehow.
I can fix this tonight if you want or we wait.
There was a problem hiding this comment.
@sjpadgett , noted that issue on multiple addresses on testing and think may have a solution (still testing it).
|
Currently testing well and confirmed the addresses are all still sent in the fhir requests. Since not super confident on the strategy i used, will await further input before i bring this into codebase (gonna postpone 7.0.0 release until likely tues/wed). |
|
@bradymiller @sjpadgett @adunsulag Please see Contact ID Fix 2 #5611. I did some modifications to Brady's fix. And, it tested well for me. Please consider it. It is not a lot of code, but it was a lot of time. |
There was a problem hiding this comment.
@bradymiller
This is working well for me and achieves our goals. Unless @adunsulag has issue which the way this is done, I say merge and let's move on.
Thanks for the work.
|
I'm going to merge this fix. I've tested add, delete, updates and views with good results. @psoas I know what it is like to want to see your contribution to the end and your vision implemented however, we really need to move on and this fix, while maybe not the way you'd like to see it done, allows us to do that. |
|
@sjpadgett I can't think of any other way to do it other than how Brady did it. All of my wasted time. My addition, although not necessary for 7.0.0 release, could be useful for avoiding excess database writes. But, I don't know if that matters. Does it? |
It is never a waste of time IMO. We all have our moments and can't catch everything. That is what peer review is all about and working as a team. So I suspect through all of this you've learned something. I know I do all the time.
I'd say it does and I'm sure you have additional development for your project where you can tune and address data management going forward. I look forward to see what's next. |
* contact id fix * only allow 1 contact per foreign table name and foreign id * getting closer * minor fix (cherry picked from commit b9e0d54)
|
@bradymiller I cherry picked this to v7 branch. I'm not seeing anything else unless there's something else I can help with for release. |
|
thanks for bringing it in. will rebuild the v7 packages tonight and likely
get release out tomorrow after test them (at this point just testing to
ensure install/upgrade in packages works and don't expect any surprises
there)
…On Mon, Jul 18, 2022 at 10:09 AM Jerry Padgett ***@***.***> wrote:
@bradymiller <https://github.com/bradymiller> I cherry picked this to v7
branch. I'm not seeing anything else unless there's something else I can
help with for release.
—
Reply to this email directly, view it on GitHub
<#5610 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACEDODXPVDFPPANFMXK3PLVUWFUDANCNFSM53ZPR4RQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
fixes #5609