> A note about the test: there's a factory.makeDivergedTranslationMessage for
> creating diverged messages.
There is, but we want this to be a *current* diverged message. makeCurrentTranslationMessage calls makeDivergedTranslationMessage when diverged=True.
> About the fix itself: approveSuggestion could be used very intensively, so for
> performance it may be worth shortcutting the getCurrentTranslation call (which
> queries the database) in cases where the message is diverged. In those cases,
> there's no chance of it being shadowed by another diverged message.
True. This change was obviously about correctness over performance.
Note that _setTranslation also calls getCurrentTranslation, so if I was optimising, I would first change _setTranslation to handle the case where the incumbent is the suggestion. Doing the check in approveSuggestion smells like Look-Before-You-Leap.
> A note about the test: there's a factory. makeDivergedTra nslationMessage for
> creating diverged messages.
There is, but we want this to be a *current* diverged message. makeCurrentTran slationMessage calls makeDivergedTra nslationMessage when diverged=True.
> About the fix itself: approveSuggestion could be used very intensively, so for lation call (which
> performance it may be worth shortcutting the getCurrentTrans
> queries the database) in cases where the message is diverged. In those cases,
> there's no chance of it being shadowed by another diverged message.
True. This change was obviously about correctness over performance.
Note that _setTranslation also calls getCurrentTrans lation, so if I was optimising, I would first change _setTranslation to handle the case where the incumbent is the suggestion. Doing the check in approveSuggestion smells like Look-Before- You-Leap.