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

Fri Mar 16 14:41:13 UTC 2012

On 03/13/2012 10:54 PM, Endi Sukma Dewata wrote:
> On 3/9/2012 7:16 AM, Petr Vobornik wrote:
>> When an error which caused calling of report_error occur, the content of
>> a facet got replaced by error message. There was no way how to force the
>> facet to recreate its content and the facet became unusable.
>>
>> This patch creates a container for an error message. On error,
>> report_error writes its content to error container, content container is
>> hidden and error container is shown. Older comment in a code suggested
>> to move the error message to facet's footer. A message in a footer could
>> be missed by the user and on top of that a footer is sometimes used by
>> various facet and we would have to solve the same problem again.
>>
>> From experience the cause of an error is usually a missing pkey in a
>> path. Therefore error information suggests user to navigate to top
>> level. It causes to load default facets with default values so errors in
>> navigation state shouldn't happen.
>>
>> Facet content is displayed back on facet_show. If user tries to display
>> same object as before facet's need_update() would return false,
>> therefore need_update was modified to always return true if error is
>> displayed.
>>
>> Also it may be possible to display facet content on refresh success. I
>> tried to avoid that because it would require putting show_content calls
>> to various success handlers or load methods. It would add one more thing
>> developer needs to think of when overriding refresh or load method.
>>
>> Reproduction:
>> 1) display any nested entity - ie DNS record
>> 2) delete its parent pkey from path - &dnszone-pkey=example.com
>> 3) reload the page with this path
>>
>> https://fedorahosted.org/freeipa/ticket/2449
>
> 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
>
> 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.
> * 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.

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

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0106-1-Content-is-no-more-overwritten-by-error-message.patch
Type: text/x-patch
Size: 9663 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120316/c62f20a1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0111-Show_content-on-refresh-success.patch
Type: text/x-patch
Size: 3127 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120316/c62f20a1/attachment-0001.bin>