OpenMRS Trunk

Patch for adding sort weight to ConceptAnswer

FYI: If you attach it as a .patch or .diff file then trac will show it with all those pretty colors and layout.

OK, i'll upload patches with .patch from now on. Thanks for the tip.

FYI about the patch – it really didn't take a lot of coding to add this feature. The only semi-arguable item is in the update-to-latest SQL script, in which I included a script that populates the new sort_weight column with the natural ordering of all existing conceptAnswers. So, everyone will still see conceptAnswers in the same order as they always did, its just that this ordering is now reflected by the sort weight. This could be removed, however, i suppose.

Beyond this, reviewing this code should be easy, as there isn't all that much to look at.

thanks.
--d

disregard ConceptAnswer.patch, its full of html garbage. Sorry.

Can trac be configured to allow me to download the patch? Or even copy out of the patch view window without picking up the line numbers?

ConceptAnswer.patch

There is an "original format" link at the bottom you can use (trac follows this pattern elsewhere, so if you find yourself in a similar predicament, check the bottom of the page).

thanks. all better now.

Can you delete the .txt attachment? Is that just an older version?

The patch looks mostly good. There are some changes needed though:

1. Only change the update-to-latest.sql file with your change. The other sql files are only updated (automatically) when a new release is made.
2. You'll want some foreign keys on that temporary table. Otherwise it will be really slow for people with a lot of concepts. (there are some people that have >60K). You can accomplish this by foreign keying as well.
3. Can you add some comments before each sql?
4. FYI: Since its ordering by concept_answer_id right now, you could just set all the weights to the concept_answer_id value.
5. This should be schema update 1.4.0.02 (after mike changes his latest commit to be 1.4.0.01)
6. Thanks in advance for adding unit tests for all of your touched methods!

Notes from Code Review 6/Oct/2008:

1. Reiterated points 1 and 4 from above comment
2. ConceptAnswer should implement Comparable<ConceptAnswer>
3. Concept.addConceptAnswer(ConceptAnswer) seems like it won't work as written because the underlying collection is a Set and not a List. Instead do something like:

   double highest = Double.MIN_VALUE;
      for (Answer a : existingAnswers)
          highest = Math.max(highest, a.sortWeight);
      newAnswer.setSortWeight(highest + 1);
      existingAnswers.add(newAnswer);

4. ConceptAnswersEditor (as well as Concept) should not change ConceptAnswer.sortWeight unnecessarily, because this will lead to sync seeing a whole bunch of extra db edits. Instead, when iterating over the answers, just ensure that they're in ascending order. If they are, leave them be, if not, then change them.

Marking this ticket as "Reviewed" and is awaiting changes to patch as laid out in previous comment by Darius.

Bumping this to 1.6. Dave, do you have a guess of when/if you'll be able to get to this? If it isn't in the near future its not a problem, we can just assign implementing the code review changes to someone else.

From dthomas:
"""
I don't need it right now, and i also don't see myself getting to this relatively soon. The patch that's attached to the ticket just needs a little more work, so if its more likely to be completed if it does get reassigned, feel free to do so, and i'll keep an eye on it.
"""

Unassigning Dave from this ticket.

Marking as minor because it's not strictly required for 1.6 release.

Marking this as Needs Work so that anyone can take this on and do those last few changes to the patch.

== Comments from the code review ==

1. In the file 'liquibase-update-to-latest.xml', update the sort weight column to be none zeros
2. In the class Concept.java you should deprecate the method getSortAnswers instead of deleting it
3. Change the modifier for the ConceptAnswerComparator class to protected
4. Effect point 4 from darius on the comments on the ticket

fixed all comments from the review

From http://openmrs.org/wiki/2010-04-05_Code_Review:

• Change concept_answer liquibase update to be an update element instead of a sql element - http://www.liquibase.org/manual/update_data
• Remove new deprecated Concpet.setAnswers(Collection, boolean)
• Remove ConceptAnswerComparator...just use Collections.sort on the list of comparables (ConceptAnswer)
• Add a sentence or two of comments before ConceptAnswersEditor startIdx logic
• Add unit test that touches ConceptAnswersEditor.setAsText
• trivial comment: (double)1 == 1d

You can commit this patch to trunk following these fixes.

Adds sort weight to ConceptAnswer

committed rev:12888