Call-in Information

Call+1-712-432-0075* or Skype a voice call to freeconferencing
Enter the code*305801

Screen sharing

ALMOST APPROVED BUT NEEDS CLARIFICATION

comment on attached patch says you were going to make a change on liquibase demo data, but that's not in the patch. Was this intentional?

Also:

verify that for ruby compatibility we can have a composite primary key on mapping tables like role_role, role_privilege, user_role

NEEDS WORK

localHeader.jsp:

url should be 'auditField.form' or something**

openmrs-servlet.xml

AuditFormFieldController.java

need (brief) javadoc on the class
47: change to 'private static final Log log ...'
delete 49-52
annotation of 53 should go under the javadoc
75, 80: internationalize these with Context.getMessageSourceService()...
85-88: move those into the end try and catch blocks
95-121: you don't need a form backing object (certainly not of type form) if you're doing all forms at once. Also remove commandName property in openmrs-servlet.xml

FormService.java

Be more verbose about what 'similar' means
Get rid of extra Map import
Get rid of @param form
Need to add an @should annotation – see http://openmrs.org/wiki/Unit_Testing_with_@should

auditFormFields.jsp

3: should be "Manage Forms", and change redirect to 'form.list' (I think)
10: internationalize in messages.properties
16: internationalize
Description of what is about to be done should be on the page. Confirm dialog can just say "Operation cannot be reversed"
It would be nice (but maybe not required) to show which or how many fields will be merged on this page.

FormDAO

292: fix comment
297: method name should be plural, i.e. getFormFieldsByField
Generally: do control-shift-F to format code. (make sure you have the OpenmrsFormatter installed)

FormServiceImpl

20, 24: imports not required
714: should be a Set not a List
720: comparison of IDs should not be required (but if it is, call the same id method for i and j)
742: change method name to be 'similar', 'match', 'equivalent', not 'equal'.
744: if any of these could be null, use OpenmrsUtil.nullSafeEquals(...)
don't say "if (X) return true else return false;" just say return (X).
you could speed up this method to be O instead of O(n^2) by looping over the field list once, making a unique string for each, and putting that in a map. Then loop once over the map to see what buckets have more than one thing. (I don't know how slow this gets with large numbers of fields, so it may not be worth changing.)
Also need to
concept/table_name/attribute_name/default_value need to be the same too
never merge and unretired field into a retired one--do it the other way

FormServiceTest.java

either add the new field & form_field in the existing FormServiceTest-formFields.xml or else create your own test dataset and do this via xml.

APPROVED with instructions for committer
messages.properties

ObsValidator.java