OpenMRS Trunk

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.

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

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.

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

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.

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.

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?

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

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.

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

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?

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.

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()]);

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.

Hey!

I have submitted a new patch implementing comments made.

Cheers

Looking good Sam!

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

New patch submitted.

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.

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

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.

Ben

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

A new patch submitted 24th of September.

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.

A patch implementing comments of the core review

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

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.

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

Created new patch based on notes from the latest code review

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

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.

Fixed the weird character issue and uploaded the latest patch.

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.

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.

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.

Patch applied in changeset rev:6445.

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.