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

fix: Field repetitions have incorrect key assigend #181

Open
jotafilaj opened this issue Jan 19, 2025 · 10 comments
Open

fix: Field repetitions have incorrect key assigend #181

jotafilaj opened this issue Jan 19, 2025 · 10 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested

Comments

@jotafilaj
Copy link

jotafilaj commented Jan 19, 2025

Search terms

  • hl7 parser

Environment

  • HL7 Client version: 3.0.0
  • TypeScript version: 5.2.2
  • Node.js version: v20.18.0
  • OS: Ubuntu 24.04

Description

While accessing fields of an HL7 segment with field repetitions, the field repetition key is the same as the parent key.

Expected Behavior

Field repetition key to be assigned correctly.

Actual Behavior

Field repetition has the same key as their parent.

Steps to reproduce the bug

const message = new Message({ text: 'MSH|^~\&|HUBWS|46355||DAL|202412091132||ORM^O01|MZ54932|P|2.3|
PID|1|999-99-9999|CHART^^^CID~MEDICAL^^^MRN||PTLASTNAME^PTFIRSTNAME^||19750825|F|||4690 MAIN STREET^^MASON^OH^45040||^^^^^513^5550124||||||999654321||' })

The problem relies on the CHART^^^CID~MEDICAL^^^MRN children. This field has the correct key according to the HL7 standards.

While its children: CHART^^^CID, and MEDICAL^^^MRN have the same key as the parent.

Sub-children CHART, CID and MEDICAL, MRN are set correctly.

@jotafilaj jotafilaj added the bug Something isn't working label Jan 19, 2025
@jotafilaj jotafilaj changed the title fix: fix: Field repetitions have incorrect key assigend Jan 19, 2025
@Bugs5382 Bugs5382 self-assigned this Jan 20, 2025
@Bugs5382
Copy link
Owner

On it!

@Bugs5382 Bugs5382 mentioned this issue Jan 22, 2025
10 tasks
@Bugs5382
Copy link
Owner

I am writing unit tests to solve this issue first, but I will get there.

@Bugs5382
Copy link
Owner

So this is a confirmed bug. I have to dig into why. If you want to help, create your own PR and suggest changes or work on my branch.

@Bugs5382
Copy link
Owner

Bugs5382 commented Jan 26, 2025

So this will fix #14 at this point. Closing #14 and #33

@Bugs5382
Copy link
Owner

Bugs5382 commented Jan 26, 2025

@jotafilaj Ok. This was a weird one, but let me explain. I hadn't for the longest time to get 3 unit tests work work by creating a way for writing/building the full path.

For example:

message.set("PV1.7").set(0).set("PV1.7.2", "Jones").set("PV1.7.3", "John");
message.set("PV1.7").set(1).set("PV1.7.2", "Smith").set("PV1.7.3", "Bob");
expect(message.toString()).toContain("PV1|||||||^Jones^John~^Smith^Bob");

Should be the equivalent of PV1.7.0.2, etc. It has always eluded me. So I commented it out. To get the unit tests to work in:

test("...should be able to set repeating fields", async () => {
// @todo this needs to be a valid way, but right now it's not working
// message.set("PID.3").set(0).set("PID.3.1", "abc");
// message.set("PID.3").set(0).set("PID.3.5", "MRN");
// message.set("PID.3").set(1).set("PID.3.1", 123);
// message.set("PID.3").set(1).set("PID.3.5", "ID");
// @todo this is a hack to get this working and passing
message.set("PID.3").set(0).set(0, "abc").set(4, "MRN");
message.set("PID.3").set(1).set(0, "123").set(4, "ID");
expect(message.toString()).toContain("PID|||abc^^^^MRN~123^^^^ID");
});
test("...can chain component setters", async () => {
// @todo this needs to be a valid way, but right now it's not working
// message.set("PV1.7").set(0).set("PV1.7.2", "Jones").set("PV1.7.3", "John");
// message.set("PV1.7").set(1).set("PV1.7.2", "Smith").set("PV1.7.3", "Bob");
// @todo this is a hack to get this working and passing
message.set("PV1.7").set(0).set(1, "Jones").set(2, "John");
message.set("PV1.7").set(1).set(1, "Smith").set(2, "Bob");
expect(message.toString()).toContain("PV1|||||||^Jones^John~^Smith^Bob");
});

I went ahead and wrote out the full chain using "set":

message.set("PID.3").set(0).set(0, "abc").set(4, "MRN");
message.set("PID.3").set(1).set(0, "123").set(4, "ID");

So the keys are being set correctly in your example:

test("...field separation", () => {
const message = new Message({ text: hl7_field_seperation });
expect(message.get("PID.1").toString()).toBe("1");
expect(message.get("PID.2").toString()).toBe("999-99-9999");
expect(message.get("PID.3").get(0).get(0).toString()).toBe("CHART");
expect(message.get("PID.3").get(0).get(3).toString()).toBe("CID");
expect(message.get("PID.3").get(1).get(0).toString()).toBe("MEDICAL");
expect(message.get("PID.3").get(1).get(3).toString()).toBe("MRN");
expect(message.get("PID.5.1").toString()).toBe("PTLASTNAME");
});

passes getting the key/child as parent.

I would need to see more of your code, but that's the only thing I find that maybe your passing code is the way that is not working fully.

@Bugs5382 Bugs5382 added the question Further information is requested label Jan 26, 2025
@Bugs5382
Copy link
Owner

In the original code that I used as a base, it works, I think. The code is so old it doesn't work and comparing my code to theirs there is no difference. So I can't say for sure it ever worked.

The idea of being able to set the "full" path in build and read should be possible. As I said, it eludes me still.

@jotafilaj
Copy link
Author

@Bugs5382 Basically what I'm trying to do is parse the hl7 message to a JSON, using a JSON mapper, I was trying to access the protected segment prop _key, I am not sure if this was the correct way, to begin with, I will try getting the fields by doing what you suggested above and let you know if it works.

@Bugs5382
Copy link
Owner

@jotafilaj I beeing trying to do that kinda for #69 so that someone could lookup "what" has been parsed. Maybe if we can combine your code, we can do it in there. I tinkerd around using JSON than then breaking it down nicely visually to the end user. It would be for debugging purposes only and not really used for production at least for #69 but I could create another modify called .toJSON() so that any segment could be broken down.

Could also be a little application CLI wise that someone could run to input tests in. 👍

Bugs5382 added a commit that referenced this issue Jan 26, 2025
* this came from a discussion
@Bugs5382
Copy link
Owner

Ref #149 into this field.

@Bugs5382 Bugs5382 added help wanted Extra attention is needed good first issue Good for newcomers labels Jan 27, 2025
@Bugs5382
Copy link
Owner

Basically, I need help solving this issue. I traced this sucker through and through and I can't figure out how to get the "paths" to be set using the String path method. So any ideas on how to fix this, would be helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants