Downloads Documentation Community Contribute Demo






Show Sidebar
Login | Register

Ticket #908 (new task)

Opened 1 year ago

Last modified 2 months ago

ConceptAnswer should have a sort weight

Reported by: dthomas Assigned to:
Priority: major Milestone: OpenMRS 1.6
Component: OpenMRS Code Base Keywords:
Cc: bwolfe., dthomas Introductory Ticket: 0
Code Review Status: Reviewed

Description

ConceptAnswer should have a sort weight. I've attached the patch.

Attachments

ConceptAnswer.txt (12.6 kB) - added by dthomas on 07/11/08 23:54:46.
Patch for adding sort weight to ConceptAnswer
ConceptAnswer.patch (12.6 kB) - added by dthomas on 07/28/08 14:43:25.
ConceptAnswer.patch

Change History

07/11/08 23:54:46 changed by dthomas

  • attachment ConceptAnswer.txt added.

Patch for adding sort weight to ConceptAnswer

07/15/08 13:09:28 changed by bwolfe

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

07/15/08 18:30:02 changed by dthomas

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

07/28/08 14:41:45 changed by dthomas

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?

07/28/08 14:43:25 changed by dthomas

  • attachment ConceptAnswer.patch added.

ConceptAnswer.patch

07/28/08 14:44:34 changed by bwolfe

  • cc set to bwolfe.
  • milestone set to OpenMRS 1.4.

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).

07/28/08 14:46:19 changed by dthomas

thanks. all better now.

07/28/08 18:19:04 changed by bwolfe

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! :-)

10/06/08 18:13:20 changed by djazayeri

  • intro_ticket changed.

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.

12/15/08 18:42:23 changed by bwolfe

  • review_status set to Reviewed.

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

12/15/08 18:42:34 changed by bwolfe

  • milestone changed from OpenMRS 1.4 to OpenMRS 1.5.

05/19/09 18:19:11 changed by bwolfe

  • milestone changed from OpenMRS 1.5 to OpenMRS 1.6.

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.

05/19/09 19:20:32 changed by bwolfe

  • cc changed from bwolfe to bwolfe., dthomas.
  • owner deleted.

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.