Downloads Documentation Community Contribute Demo






Show Sidebar
Login | Register

Ticket #890 (closed enhancement: fixed)

Opened 2 years ago

Last modified 1 year ago

Show hl7_in_error rows in the webapp

Reported by: bwolfe Assigned to: sthaiya
Priority: minor Milestone: OpenMRS 1.5
Component: OpenMRS Code Base Keywords:
Cc: jmiranda, djazayeri Introductory Ticket: 0
Code Review Status: Approved

Description (Last modified by bwolfe)

Currently there is no way for administrators to see what is in their hl7_in_error queue table. It would be nice to have a section in the webapp that displayed what was in the table. The admin should be allowed to delete queue items.

As a second bonus step, give the admin a way to move the message back into the hl7_in_queue with the click of a button.

Attachments

Ticket890.patch (46.3 kB) - added by bwolfe on 10/28/08 19:15:45.
Ticket890-New.patch (43.6 kB) - added by sthaiya on 11/25/08 06:45:25.
A patch implementing comments of the core review
890-jmiranda-17-dec-2008.patch (41.6 kB) - added by jmiranda on 12/17/08 23:15:52.
fixed patch (previous version was created as multi-project)
890-jmiranda-17-dec-2008.2.patch (41.6 kB) - added by jmiranda on 12/19/08 20:54:24.

Change History

07/28/08 13:49:07 changed by bwolfe

  • owner changed from somebody to sthaiya.

Excellent. I will review this shortly. FYI, if you attach it as a .patch or a .diff file then trac will display it nicely with colors, etc. :-)

07/29/08 06:26:02 changed by sthaiya

I attached as .patch and as you said it displays nicely with colors and beautiful format.

07/29/08 14:55:14 changed by bwolfe

  • milestone changed from OpenMRS Someday to OpenMRS 1.4.

Awesome, most of the patch looks excellent. Some minor points to address:

  1. We don't put @Author tags on the classes as its not the "open source" way. People can see who committed it or who contributed by looking at the commit comment. (for reference, this is an awesome video about why we do some things like this: http://video.google.com/videoplay?docid=-4216011961522818645&q=poisonous+people&ei=5CqPSLObDoPM4AKj6_mHCA )
  2. Why have a separate Hl7InArchiveDeleted object? Why not just have methods that give you Hl7InArchive objects that are deleted vs ones that are archived
  3. Your queue/archive form jsp pages have column headers that aren't using the spring:messages.
  4. Hl7inErrorListController has some weird class comments. looks like a simple copy/paste error.
  5. Our convention is to put the opening curly brace { on the same line as the method/function. Can you adjust your code? You can set eclipse to do it for you. See "code style" on http://openmrs.org/wiki/Conventions
  6. Why did you add errorMessage/errorState to the queue table? If you're moving something from the error table to the in queue, the error message/state should just be dropped.

07/30/08 13:42:04 changed by sthaiya

Thanks for the comments that are really eye openning!

I went through them and effected comment 1,4 and 5 as is. Tho':

In comment two: I did not create HL7InArchiveDeleted object. I added getHL7InArchiveDeleted method in the HL7InArchive object to return deleted messages vs archived ones. In comment three: do I also use spring:messages for the column headers? I just used it for page/section headers:

In comment six: The errorMessage-(error_msg) and errorState-(state) columns were existing in the queue table. I mapped them in the hbm so that I can display their contents. I feel they (esp. state column) have very important info for the admin.

Regards

09/15/08 19:59:00 changed by bwolfe

  • cc set to jmiranda, djazayeri.

Sorry about the delay, I didn't realize you had added a new attachment. For some reason trac doesn't send out notifications if you only add an attachment.

Looks good. Pending a second review from someone else this can be put into trunk.

09/15/08 20:41:42 changed by djazayeri

Hi,

One substantive comment, and one nitpicky one:

  • Why is the new column in hl7_in_archive called 'msg_source'? If the only options are 'processed' and 'deleted', then it should be called a status, not a source. Also, you don't want to hardcode the numbers 0 and 1. Ideally you should use an Enum, but at least put 0 and 1 in the OpenmrsConstants class.
    • Also, don't abbreviate: just call the column message_source. And is it supposed to be an int(11)?
  • The capitalization in this line is wrong: public class Hl7inQueueListController. in should be In.

09/16/08 10:37:39 changed by sthaiya

Hi! Thats cool Darius, I will work on the two comments. Actually it should be tinyint(1). I will also rename the column to status and work on that capitalization. Ben?

09/16/08 12:29:51 changed by bwolfe

There is already the message_state column. What does that store currently? Could we just use that column for marking?

09/16/08 12:39:52 changed by sthaiya

message_state column is in the hl7_in_queue table. The is no status column in the hl7_in_archive table. I renamed my flag column (msg_source) to state.

09/16/08 13:19:33 changed by djazayeri

Okay, so rather than a tinyint(1) how about doing a varchar(10) and put the actual words 'processed' or 'deleted' in it?

09/16/08 13:25:34 changed by sthaiya

Darius, I would suggest it remains as coded to conform to the convention in other state columns in the HL7 section. e.g HL7InQueue status. What d'ya think?

09/16/08 14:46:44 changed by sthaiya

Thanx Ben and Darius.

In this patch I have implemented comments made by Darius: 1. Capitalization 2. Renamed msg_source column to state 3. Did away with hardcoded 0 and 1 and used the HL7Constants class to have the constants.

09/16/08 14:52:57 changed by jmiranda

Hey Samuel

Sorry the late response - I tried to add this comment yesterday, but Trac was being flakey. Here are some more suggestions:

  • I agree with Darius on the column name ... messageState would seem to make more sense. Could you explain the difference between hl7_in_queue.message_state and hl7_in_archive.msg_source.
  • In addition, I agree with the use of enum or at least a constant. The following code is confusing with an enum or constant ("setMessageSource(0);").
  • You also use "int j=100" in several places. Even after reading the code, I have no idea why you chose this number. I would prefer to see this defined as a constant (CHARACTERS_PER_LINE?). Is there a purpose for using a 100 or is it random?
  • What was the purpose for breaking up the HL7 xml data? Did it display on a single line? This might be better placed in the JSP (if possible).
  • Hl7inQueueListController.referenceData(): you should define an array of error messages and then use the enum/constant to reference the correct error message (i.e. map.put("status", statusMessages[queue.getMessageState()]););

09/17/08 07:29:22 changed by sthaiya

Hey Justin:

  • Comment one is Ok, I implemented suggestions as made by Darius and backed by you. hl7_in_queue.message_state keeps a state of a message that is in the queue table. Either pending, processing, processed or error. The hl7_in_archive.msg_source now hl7_in_archive.message_state flags between processed and deleted messages - all of which are in the archives table.
  • Comment two I used HL7Constants class as suggested by darius.
  • Comments 3 & 4: Yes HL7 xml data was displaying in long lines. I just chose 100 randomly. And true should use a (CHARACTERS_PER_LINE) constant. Will work on that.
  • Comment 5: Would suggest we use messages.properties rather than array for ease of translation to other languages. What d'ya think?

I will work on the changes and submit a refined patch. Thanks all.

09/22/08 08:48:43 changed by sthaiya

  • intro_ticket changed.

Hey!

I have submitted a new patch implementing comments made.

Cheers

09/22/08 13:26:14 changed by bwolfe

Looking good Sam!

Your localHeader.jsp page needs to be rid of the double double quotes. See #1019 for more info.

09/23/08 09:08:20 changed by sthaiya

New patch submitted.

09/23/08 14:47:42 changed by bwolfe

Getting close Sam!

I think the latest changes to the column name messed up a few of your pages. I am getting an sql exception when trying to view the delete hl7 messages. I also get one when trying to delete a queue item.

Yet another nitpicky point: The links for Manage Queue Messages, Restore Deleted Messages, and Manage hl7 errors need to be bold if you are on that page. Looks like the if statements in your localheader are just off a bit for that.

09/23/08 14:59:32 changed by sthaiya

do you have the column state added in your hl7_in_archive table? That could be the reason.

09/23/08 15:10:31 changed by bwolfe

Argh, you are correct. I ran the sqldiff on the wrong database. The viewing/deleting/restoring is working correctly. The bold links in localheader are still not there though.

09/24/08 09:01:02 changed by sthaiya

Ben

Ok. I worked on the bold links in localheader and also the priveleges, now working cool. A new patch submitted.

09/29/08 07:31:45 changed by sthaiya

A new patch submitted 24th of September.

10/28/08 19:14:30 changed by bwolfe

Sam, we finally got your patch through an official review. You can see our comments in this google doc: http://docs.google.com/Doc?id=dnc9s5q_127dtwn2ndp&hl=en

Can you comment on the TODOs and/or apply them to a new patch?

I've uploaded an updated patch that applies cleanly against the latest trunk.

10/28/08 19:15:45 changed by bwolfe

  • attachment Ticket890.patch added.

11/25/08 06:45:25 changed by sthaiya

  • attachment Ticket890-New.patch added.

A patch implementing comments of the core review

11/25/08 07:06:41 changed by sthaiya

BEN:

I worked on all comments.

I moved presentation Logic from refenceData() to JSP thus did away with CHARACTERS_PER_LINE constant.

Comment #1 - HL7 to Hl7 is not fully implemented. I propose that I handle that as an addendum to the ticket since I will need to change objects used elsewhere in the system.

Regards

12/03/08 15:33:00 changed by bwolfe

  • milestone changed from OpenMRS 1.4 to OpenMRS 1.5.

Great, thanks Sam! I will take a look at it and see about moving it into core.

We can create a second ticket that does the changes to the Hl7/HL7 changes.

12/09/08 14:05:09 changed by bwolfe

  • review_status set to Needs Review.

Marking this as "Needs Review" so that the final review is covered in one of the weekly code reviews.

12/17/08 23:11:38 changed by jmiranda

  • cc changed from jmiranda, djazayeri to jmiranda, djazayeri.
  • description changed.

Created new patch based on notes from the latest code review

12/17/08 23:15:52 changed by jmiranda

  • attachment 890-jmiranda-17-dec-2008.patch added.

fixed patch (previous version was created as multi-project)

12/18/08 19:21:20 changed by bwolfe

  • description changed.

Justin, you have some weird characters in the metadata/model/update-to-latest-db.mysqldiff.sql file.

Sam, do you have anything to add to our latest comments? http://docs.google.com/View?docID=dnc9s5q_127dtwn2ndp&revision=_latest#_Ticket_890_Show_hl7_in_error_ and http://docs.google.com/View?docID=dnc9s5q_127dtwn2ndp&revision=_latest#_Ticket_890_HL7_Queue_Manageme

(Your patch was about 99% of the way there! :-) We just renamed a few of the constants to line up with the other ones)

Sam, if you approve, we can go ahead and commit this to trunk.

12/19/08 20:54:24 changed by jmiranda

  • attachment 890-jmiranda-17-dec-2008.2.patch added.

12/19/08 20:55:42 changed by jmiranda

  • cc changed from jmiranda, djazayeri to jmiranda, djazayeri.
  • description changed.

Fixed the weird character issue and uploaded the latest patch.

12/21/08 19:41:42 changed by sthaiya

Ben, Justin[[BR]] If its OK with you Justin maybe yo can have same changes on the new patch I submitted. I moved the "CHARACTERS_PER_LINE" logic from controller to jsp and used overflow and pre tags.

12/22/08 14:15:16 changed by bwolfe

  • description changed.

I'm pretty sure the patch your changes to that in it. You had only commented it out in the controllers, thats why you're still see CHARACTERS_PER_LINE in the patch file.

12/22/08 14:19:34 changed by bwolfe

  • review_status changed from Needs Review to Approved.

Justin, I think this is ready for commit. Go ahead and get rid of the large commented blocks in the controllers as your commit it.

12/23/08 00:48:49 changed by jmiranda

  • cc changed from jmiranda, djazayeri to jmiranda, djazayeri.
  • status changed from new to closed.
  • resolution set to fixed.
  • description changed.

Patch applied in changeset [6445].

12/24/08 16:37:24 changed by bwolfe

  • description changed.

Thanks for doing this (and putting up with us) Sam! Your work is very much appreciated. We're looking forward to continued great coding from you. :-)