Code review comment for lp:~henninge/launchpad/bug-482267

Hi Henning,

Your changes look good. I just have one suggestion below.

merge-approved

-Edwin

>>> + if (new_status == RosettaImportStatus.APPROVED and
>>> + not (self.import_into is not None and can_admin)):
>>
>>
>> I think it would be clearer without the double negatives.
>> if (new_status == RosettaImportStatus.APPROVED and
>> (self.import_into is None or not can_admin)):
>
>Actually, that is what I changed this line from (previously in the view
>code) because I think "and" is easier to understand here. It derives
>more directly from the definition "an entry can be approved by an admin
>but only if we have an import target".
>
>So, I like this better but if you insist, will change it back ... ;)

Here is another thought of how you can keep the "and" but get rid of the
double negative. You can take it or leave it.

    if new_status == RosettaImportStatus.APPROVED:
        if can_admin and self.import_into is None:
            pass
        else:
            return False