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

[Endi Sukma Dewata]

ACK 106-1 and 111. See comments below.

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

Or use the error as the title (without 'An error has occured'):

   IPA Error 3007

   'idnsname' is required

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

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

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.

-- 
Endi S. Dewata