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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
