[Freeipa-devel] [PATCH] 107 Content is no more overwritten by error message

[Petr Vobornik]

On 03/19/2012 04:47 PM, Endi Sukma Dewata wrote:
> ACK 106-1 and 111. See comments below.

Pushed to master, ipa-2-2.

> On 3/16/2012 9:41 AM, Petr Vobornik wrote:
>> On 03/13/2012 10:54 PM, Endi Sukma Dewata wrote:
>>> In the ticket I added 2 more scenarios to reproduce the problem. So we
>>> have 3 possible cases:
>>> 1. invalid UI state
>>> 2. non-existent entry
>>> 3. server down
>>>
>>> For case #1, the patch provides a much better message, but I think
>>> ideally if some parameters are missing from the URL (e.g. the parent
>>> pkey) it should be detected by the UI before sending the request to the
>>> server. This probably should be addressed in a separate ticket. See the
>>> note below about the error message.
>>>
>>> For case #2, the patch fixes the issue by clearing up the error message.
>>> This works on all entities except users because the user details page
>>> uses a batch operation to get the data and it doesn't handle
>>> non-existent users properly. I think this is an existing and separate
>>> issue.
>>>
>>> For case #3, the patch will show a message saying that the UI got into
>>> an invalid state, which is actually not the case here. Also, returning
>>> to the main page or reloading the page wouldn't solve the problem
>>> either.
>>>
>>> So for this ticket I think it would be better to show a more generic
>>> error message, something like this:
>>
>> Reworked.
>>>
>>> An error has occured (IPA Error 3007)
>>>
>>> 'idnsname' is required
>
> Do you think it would look better if the message is formatted like this:
>
> An error has occured
>
> IPA Error 3007: 'idnsname' is required

I'm indecisive between this and implemented.

>
> Or use the error as the title (without 'An error has occured'):
>
> IPA Error 3007
>
> 'idnsname' is required
>

With ["Unknown error","error"] it looks cryptic.

> It's up to you. Feel free to change it before push if you want.

--> I haven't changed it.
>
>>> Please try the following options:
>>> * Refresh the page. (see note below)
>>> * Return to the main page and retry the operation.
>> We are talking about main page. What is it? Identity/Users? Navigation
>> code operates also with currently displayed facet. So when I now
>> navigate to '#' (empty state) it won't have to be Identity/Users. The
>> good part is that it navigates to page in a branch where user was
>> operating.
>
> Right, the 'main page' isn't really defined, so the user might try all
> possible 'main pages' until it works again (which is not necessarily a
> bad thing). Not sure if we should use the term 'search/list page', it
> might not apply to all cases. At least the link helps reduce the confusion.
>
>>> * Reload the browser.
>>> If the problem persists please contact the system administrator.
>>>
>>> Each of the above options could be made into a link that does the
>>> mentioned operation.
>>>
>>> It would be great if we can use the Refresh button to clear the error
>>> message. If this requires significant effort we probably can remove this
>>> option from the message above and add it in a separate ticket.
>>
>> The patch is quite short. I was more concerned about the fact that when
>> overriding stuff, developer would have to think about one more thing.
>> The UI is getting more and more complex. But it might not be as a big
>> problem as I originally thought. I put the call to refresh success
>> handler, mainly because report_error is in error handler, and these
>> handlers aren't overridden often. Attached as separate patch.
>
> OK. The refresh/load/reset/update is one area that still needs cleanup.
>
>>> One more thing, this may not be a problem now, but the error_container
>>> uses both facet-content and facet-error CSS classes. I understand this
>>> is done to avoid code duplication, but this also means the facet will
>>> have 2 facet-contents. CSS classes can be used for decorative or
>>> structural purposes or both, so we need to make sure decorative changes
>>> will not affect it structurally. One solution is to duplicate the CSS
>>> code from facet-content into facet-error. Another solution is to use a
>>> separate decorative class that are added into both facet-content and
>>> facet-error elements.
>>>
>> It is little bit more difficult. If I look at it structurally the error
>> div is facet-content too. So the facet has two contents - proper and
>> error. Is it OK? Does it break some design principle. If so, would it be
>> better to have separate error_facet?
>>
>> I think it is not good to have two contents but current implementation
>> is not ready for separate error_facet - navigation code might protest.
>
> My original concern was suppose someone uses jQuery to search for
> facet-content he might find 2 elements. But maybe not since only one of
> them is enabled, I haven't verified it. I think this is OK for now.
> We'll revisit the code if it becomes a problem.

What about ids? ie: id="entity_name-facet_name-content_name"

>
> One more thing, try shutting down slapd and load the UI. After closing
> the error dialog the page will show the same error message, but it isn't
> nicely formatted. I think it should show the same message like above
> probably with reloading the browser as the only option. Or the refresh
> option too if we show the Refresh button. This can be fixed separately.
>


-- 
Petr Vobornik