On 08/26/2011 11:04 PM, Endi Sukma Dewata wrote:
On 8/26/2011 11:41 AM, Petr Vobornik wrote:
https://fedorahosted.org/freeipa/ticket/1689
Currently adding or deleting sudo options will refresh the entire page.
It's not a problem but the code could be optimized to refresh only the
sudo options table
We have several scenarios for sudo options:
1. Add succeeded: The command returns the new record, so we can use it
to load the table. No problem here.
2. Add failed: We may be able to assume the data on the server didn't
change, so we don't have to update the table. (Yes, the old code does a
refresh, but I don't think it's necessary.)
In most cases it's true. One scenario, when update could be useful is
when there is an network error while receiving response. But in this
case updating the table would probably ended with the same error.
Another case is when someone added the same option right before you.
3. Delete batch failed: I think we can assume nothing was executed, same
as #2.
This time only some network issue can occur.
4. Delete batch succeeded: It could contain a mix of successes and
failures. Like you said, we should use the last successful result.
But instead of checking only the last result and do a load() or
update(), we could iterate through the results and find the last
successful one (the one with non-empty result).
Updated
If we find one, then we can use it to load the table. If there isn't
any, it means all failed, so we don't do anything, same as #2.
Same as 2 only for delete operation - you'll end with invalid table in
concurrent deletion.
What do you think?
Summary:
I would say that the network issue and the same concurrent edit issue
can be so rare, that the update isn't much necessary and it slows down
more frequent failures like non-concurrent adding of the same option.
If we want UI to be faster, we should removed updates. If we want it to
be more valid in rare cases we should keep updates.
Included updated patches for both options (1-without updates, 2-with
updates).
--
Petr Vobornik
From a3131924fd5513b724864f5661c239fe93a93899 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Fri, 26 Aug 2011 18:36:54 +0200
Subject: [PATCH] Modifying sudo options refreshes the whole page
https://fedorahosted.org/freeipa/ticket/1689
Currently adding or deleting sudo options will refresh the entire page. It's not a problem but the code could be optimized to refresh only the sudo options table
---
install/ui/sudo.js | 23 +++
install/ui/widget.js | 10 +-
2 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/install/ui/sudo.js b/install/ui/sudo.js
index 8d33550c0d7c25bc8f882084cb8a04662a54ebd7..18dd024f5eec9be21d76218f1e3ba95dc6e7a618 100644
--- a/install/ui/sudo.js
+++ b/install/ui/sudo.js
@@ -574,12 +574,11 @@ IPA.sudo.options_section = function(spec) {
options: {
ipasudoopt: value
},
-on_success: function() {
-that.facet.refresh();
+on_success: function(data) {
+that.load(data.result.result);
dialog.close();
},
-on_error: function() {
-that.facet.refresh();
+on_error: function(data) {
dialog.close();
}
});
@@ -618,12 +617,20 @@ IPA.sudo.options_section = function(spec) {
dialog.execute = function() {
var batch = IPA.batch_command({
-on_success: function() {
-that.facet.refresh();
+on_success: function(data) {
+//last successful result of batch results contains valid data
+var result;
+for(var i = data.result.results.length - 1; i -1; i--) {
+result = data.result.results[i].result;
+if(result) {
+that.load(result);
+break;
+}
+}
+
dialog.close();
},
-on_error: function() {
-that.facet.refresh();
+on_error: function(data) {
dialog.close();
}
});
diff --git a/install/ui/widget.js b/install/ui/widget.js
index 62af6c16d10aac65e51191f2da955b8f1ebb3bed..83cb4dcb23c6a296739bf7e8604ef3f7a6a5b3e7 100644
--- a/install/ui/widget.js
+++ b/install/ui/widget.js
@@ -1471,11 +1471,11 @@ IPA.table_widget = function (spec) {
that.empty();
that.values = result[that.name];
-if (!that.values) return;
-
-for (var i=0; ithat.values.length; i++) {
-var record = that.get_record(result, i);
-that.add_record(record);
+if (that.values) {
+for (var i=0;
