2010-04-15 Code Review

Owned by Former user (Deleted)
Last updated: Aug 18, 2010Version comment
2 min read

Call-in Information

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

Screen sharing

Attendance

  • Ben Wolfe

  • Jeremy Keiper

  • Sy Haas

  • Win Ribeka

  • Wyclif Luyima

  • Darius Jazayeri

  • Dawn Smith

  • Paul Biondich (observing)

  • Burke Mamlin

Tickets to review

Notes for ticket 2198

ModuleFactory #428-439 should be in*checkRequiredVersion (specifically the null check)

  • ModuleFactory #437: no need to catch the exception, just call checkRequiredVersion and let the exception pass through

  • ModuleUtil #245: remove comment and add better description to javadoc

  • ModuleUtil #320: remove the !(not) and change logic to be simpler (i.e. !(a && b) = !a || !b)

  • ModuleUtil #321-322, #325-326: move error messages into message source service

Notes for ticket 2240

  • use @Deprecated annotation for deprecated methods (as well as @deprecated in the javadoc)

Notes for ticket 2232

  • need to set the UUID as not-null in fourth changeset

  • roll changes into User changeset

  • roll those changes into whatever Darius copied from

Notes for ticket 2189

  • deprecate it: 2, leave it: 2, revert it: ALL

  • PersonService: do not add another method to PersonService, just modify the functionality to return retired along with non-retired

Notes for ticket 282

CTRL-SHIFT-O to remove . imports

  • CTRL-SHIFT-F to fix tabbing for new text

  • messages.properties: use general.none instead of noUsers?

  • OpenmrsConstants #468: use VIEW_USERS privilege instead of adding a new constant

  • OpenmrsConstants #1308-1309: should be in WebConstants

  • LoginServlet #255: use StringUtils.equals (null-safe)

  • LoginServlet #256: use "systemid:" + currentUser.getSystemId() instead of "admin"

  • refactor enQueue and deQueue methodology

  • CurrentUsersController #52: alphabetize the usernames

  • currentUsers.jsp #18: it will always be zero ... take out the class

  • currentUsers.jsp #19: change to general.none instead of noUsers (Remove from messages.properties)

TODOs for us

  • make a wiki page for common best practices we try to follow within OpenMRS

    • i.e. use org.apache.commons.lang.StringUtils#equals(String, String) instead of .equals()

    • make sure we're clear about returning retired data in get___ByName() methods

      • UI does not necessarily want to see retired data

      • HL7 processing should see retired data

      • should we be writing service methods with includeRetired parameters along with includeVoided?

  • make a unit test that looks at all OpenMRS objects and check that UUIDs are actually mapped to the database

  • start a wiki page that keeps track of things we change that are not backwards compatible