[Freeipa-devel] [PATCH] 0026 Do not display success message on failure in web UI

[Petr Vobornik]

On 05/16/2013 04:26 PM, Ana Krivokapic wrote:
> On 05/16/2013 01:20 PM, Petr Vobornik wrote:
>> On 05/15/2013 05:43 PM, Ana Krivokapic wrote:
>>> On 05/13/2013 04:51 PM, Petr Vobornik wrote:
>>>> On 05/07/2013 05:16 PM, Ana Krivokapic wrote:
>>>>> https://fedorahosted.org/freeipa/ticket/3591
>>>>>
>>>>
>>>> 1) The change from on_success to on_error is causing problems when
>>>> some command in a batch doesn't fail. Ie.: disable multiple users on
>>>> user search facet. Disabling already disabled user causes an error.
>>>> The dialog is shown but the page is not refreshed so the newly
>>>> disabled records are still displayed as enabled. We might even call
>>>> this case a success.
>>>>
>>>> IMO we shouldn't change the method because the batch itself succeeded.
>>>> The problem should be fixed on caller side (users of batch command).
>>>>
>>>> 2) Also `ajax` context should be left there instead of `this`,
>>>> otherwise it would get the context of on_ok handler:
>>>>
>>>> 3) (not an actual issue) Some of my old code doesn't contain space
>>>> between for/if and opening curly bracet, opposite to the rest of the
>>>> Web UI. Spaces should be added when touching these parts of code.
>>>
>>> Since the problem occurs in the case when the batch succeeds, but some
>>> commands from the batch fail, it should be enough to modify the message
>>> that is displayed. I modified it so it shows exactly how many items from
>>> the batch succeeded.
>>>
>>> Updated patch is attached.
>>>
>>
>> It's a good direction.
>>
>> 1) Search trough the code revealed that there are also other cases:
>> a) in association_facet:association.js:1031,1082 - add and delete when
>> bulk_associator (default) is used (ie. in group/member_user). Make
>> sure that it doesn't crash when serial associator is used (it doesn't
>> use batch command).
>> b) sudo rule delete option
>>
>> Can be reproduced by using two Web UIs and trying to do the same
>> action in both instances.
> Fixed in updated patch. I also slightly reworded strings in internal.py,
> to avoid situations like "1 items were added".
>
>>
>> 2) I wonder whether to not call notify_success when all commands in a
>> batch fail. User should know what failed from the error dialog. But it
>> is not sufficiently verbose in this matter, it should show how many
>> commands succeeded and failed -
>> https://fedorahosted.org/freeipa/ticket/1702 . Until then I'm hesitant
>> to omit the notify_success call. On the other hand, it might be easier
>> to do now. What do you think?
>>
>
> I would not omit it - I think the displayed message is quite clear and
> no longer misleading. It is an extra piece of information for the user
> and could be useful in the case when all commands in a batch fail, as it
> is not immediately clear from the error dialog that this is what
> happened. I would leave it in for now, at least until we see what the
> solution for #1702 will look like.
>

ACK and pushed to master, ipa-3-2.
-- 
Petr Vobornik