launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02731
[Merge] lp:~wallyworld/launchpad/recipe-request-build-partial-success into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/recipe-request-build-partial-success into lp:launchpad with lp:~wallyworld/launchpad/recipe-build-now as a prerequisite.
Requested reviews:
Launchpad UI Reviewers (launchpad-ui-reviewers): ui
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#722893 Recipe build request popup behaves oddly on partial success
https://bugs.launchpad.net/bugs/722893
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/recipe-request-build-partial-success/+merge/50873
Provide better user experience when requesting recipe builds where some work and others fail (partial success).
== Implementation ==
Change the form action so that when there is an error, the json data structure returned to the client contains the successful form rendering and the errors. The form html is used to update the page and the errors are displayed, along with a message about the successful builds.
== Demo and QA ==
Attempt to request some new recipe builds and select a distro where there isn't a build pending and another distro where there is. The form should display an informational message about the successful builds and an error message about the others.
See screenshot: http://people.canonical.com/~ianb/request-build-partial-success.png
== Tests ==
Added to the existing windmill test for requesting builds
bin/test -vvt test_recipe_request_build
== Lint ==
Linting changed files:
lib/lp/code/browser/sourcepackagerecipe.py
lib/lp/code/javascript/requestbuild_overlay.js
lib/lp/code/templates/sourcepackagerecipe-index.pt
lib/lp/code/windmill/tests/test_recipe_request_build.py
./lib/lp/code/javascript/requestbuild_overlay.js
102: Line exceeds 78 characters.
172: Line exceeds 78 characters.
296: Line exceeds 78 characters.
--
https://code.launchpad.net/~wallyworld/launchpad/recipe-request-build-partial-success/+merge/50873
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/recipe-request-build-partial-success into lp:launchpad.
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2011-02-23 05:29:00 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2011-02-23 05:29:01 +0000
@@ -374,8 +374,14 @@
data['archive'], self.user, distroseries, manual=True)
builds.append(build)
except BuildAlreadyPending, e:
- errors['distros'] = ("An identical build is already pending "
- "for %s." % e.distroseries)
+ existing_error = errors.get("distros")
+ if existing_error:
+ new_error = existing_error[:-1] + (
+ ", and %s." % e.distroseries)
+ else:
+ new_error = ("An identical build is "
+ "already pending for %s." % e.distroseries)
+ errors["distros"] = new_error
return builds, errors
@@ -487,7 +493,8 @@
"""Set up the response and json data to return to the caller."""
self.request.response.setStatus(400, reason)
self.request.response.setHeader('Content-type', 'application/json')
- return simplejson.dumps(errors)
+ return_data = dict(builds=data, errors=errors)
+ return simplejson.dumps(return_data)
def failure(self, action, data, errors):
"""Called by the form if validate() finds any errors.
@@ -510,10 +517,14 @@
"""
builds, errors = self.requestBuild(data)
# If there are errors we return a json data snippet containing the
- # errors instead of rendering the form. These errors are processed
- # by the caller's response handler and displayed to the user.
+ # errors as well as the form content. These errors are processed
+ # by the caller's response handler and displayed to the user. The
+ # form content may be rendered as well if required.
if errors:
- return self._process_error(data, errors, "Request Build")
+ builds_html = None
+ if len(builds):
+ builds_html = self.render()
+ return self._process_error(builds_html, errors, "Request Build")
@property
def builds(self):
@@ -696,7 +707,7 @@
def initialize(self):
super(SourcePackageRecipeAddView, self).initialize()
if getFeatureFlag(RECIPE_BETA_FLAG):
- self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
+ self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
widget = self.widgets['use_ppa']
current_value = widget._getFormValue()
self.use_ppa_existing = render_radio_widget_part(
@@ -728,8 +739,8 @@
# Grab the last path element of the branch target path.
source = getUtility(ISourcePackageRecipeSource)
for recipe_name in self._recipe_names():
- if not source.exists(owner, recipe_name):
- return recipe_name
+ if not source.exists(owner, recipe_name):
+ return recipe_name
@property
def initial_values(self):
@@ -737,7 +748,7 @@
series = [series for series in distroseries if series.status in (
SeriesStatus.CURRENT, SeriesStatus.DEVELOPMENT)]
return {
- 'name' : self._find_unused_name(self.user),
+ 'name': self._find_unused_name(self.user),
'recipe_text': MINIMAL_RECIPE_TEXT % self.context.bzr_identity,
'owner': self.user,
'distros': series,
=== modified file 'lib/lp/code/javascript/requestbuild_overlay.js'
--- lib/lp/code/javascript/requestbuild_overlay.js 2011-02-23 05:29:00 +0000
+++ lib/lp/code/javascript/requestbuild_overlay.js 2011-02-23 05:29:01 +0000
@@ -29,7 +29,7 @@
RequestResponseHandler = function () {};
RequestResponseHandler.prototype = {
clearProgressUI: function () {},
- showError: function (error_msg) {},
+ showError: function (header, error_msg) {},
getErrorHandler: function (errorCallback) {
var self = this;
return function (id, response) {
@@ -37,12 +37,13 @@
// If it was a timeout...
if (response.status == 503) {
self.showError(
- 'Timeout error, please try again in a few minutes.');
+ 'Timeout error, please try again in a few minutes.',
+ null);
} else {
if (errorCallback != null) {
errorCallback(self, id, response);
} else {
- self.showError(response.responseText);
+ self.showError(response.responseText, null);
}
}
};
@@ -95,9 +96,26 @@
request_build_response_handler.clearProgressUI = function() {
destroy_temporary_spinner();
};
- request_build_response_handler.showError = function(error) {
- request_build_overlay.showError(error);
- Y.log(error);
+ request_build_response_handler.showError = function(header, error_msgs) {
+ if (header == null) {
+ if (error_msgs == null)
+ header = "An error occurred, please contact an administrator.";
+ else
+ header = "The following errors occurred:";
+ }
+ var error_html = header;
+ if (error_msgs != null) {
+ error_html += "<ul>";
+ if (typeof(error_msgs) == "string"){
+ error_msgs = [error_msgs];
+ }
+ Y.each(error_msgs, function(error_msg){
+ error_html += "<li>" + error_msg.replace(/<([^>]+)>/g,'') +
+ "</li>";
+ });
+ error_html += "</ul><p/>";
+ }
+ request_build_overlay.error_node.set('innerHTML', error_html);
};
};
@@ -129,7 +147,7 @@
request_daily_build_handle.removeClass("unseen");
var server_error = 'Server error, ' +
'please contact an administrator.';
- handler.showError(server_error);
+ handler.showError(server_error, null);
}),
success:
request_daily_build_response_handler.getSuccessHandler(
@@ -171,9 +189,12 @@
request_daily_build_response_handler.clearProgressUI = function() {
destroy_temporary_spinner();
};
- request_daily_build_response_handler.showError = function(error) {
- alert(error);
- Y.log(error);
+ request_daily_build_response_handler.showError = function(header, error) {
+ var error_msg = header;
+ if (error != null)
+ error_msg += " " + error;
+ alert(error_msg);
+ Y.log(error_msg);
};
};
@@ -222,7 +243,7 @@
if (Y.Object.size(distros) == 0) {
request_build_response_handler.showError(
"You need to specify at least one distro series for " +
- "which to build.");
+ "which to build.", null);
return false;
}
return true;
@@ -257,11 +278,28 @@
request_build_overlay = null;
return;
}
- var error_info = Y.JSON.parse(response.responseText)
+ var build_info = Y.JSON.parse(response.responseText);
+ var build_html = build_info['builds'];
+ var error_info = build_info['errors'];
var errors = [];
+ var error_header = null;
+ if (build_html != null) {
+ var nr_new = display_build_records(
+ build_html, current_builds);
+ var new_builds_message = ONE_BUILD_MESSAGE;
+ if (nr_new > 1) {
+ new_builds_message =
+ Y.Lang.substitute(
+ MANY_BUILDS_MESSAGE,
+ {nr_new: nr_new});
+ }
+ error_header = "<p><div class='popup-build-informational'>"
+ +new_builds_message+"</p></div>"+
+ "There were also some errors:";
+ }
for (var field_name in error_info)
errors.push(error_info[field_name]);
- handler.showError(errors);
+ handler.showError(error_header, errors);
}),
success: request_build_response_handler.getSuccessHandler(
function(handler, id, response) {
=== modified file 'lib/lp/code/templates/sourcepackagerecipe-index.pt'
--- lib/lp/code/templates/sourcepackagerecipe-index.pt 2011-02-23 05:29:00 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-index.pt 2011-02-23 05:29:01 +0000
@@ -23,11 +23,18 @@
color: black;
padding: 5px 5px 5px 5px;
margin-right: 40px
- }
+ }
.build-informational::before {
padding-right: 5px;
content: url('/@@/info');
- }
+ }
+ .popup-build-informational {
+ color: black;
+ }
+ .popup-build-informational::before {
+ padding-right: 5px;
+ content: url('/@@/info');
+ }
</style>
</metal:block>
=== modified file 'lib/lp/code/windmill/tests/test_recipe_request_build.py'
--- lib/lp/code/windmill/tests/test_recipe_request_build.py 2011-02-23 05:29:00 +0000
+++ lib/lp/code/windmill/tests/test_recipe_request_build.py 2011-02-23 05:29:01 +0000
@@ -77,11 +77,21 @@
% quote_jquery_expression(PPAFormatterAPI(self.ppa).url()),
timeout=FOR_ELEMENT)
- # And try the same one again.
+ # And try the same one again plus a new one.
client.click(id=u'request-builds')
client.waits.forElement(id=u'field.archive')
+ client.click(id=u'field.distros.1')
client.click(name=u'field.actions.request')
+ # And check that there's an info message.
+ client.waits.forElement(
+ jquery=u"('div.popup-build-informational')",
+ timeout=FOR_ELEMENT)
+
+ client.asserts.assertTextIn(
+ jquery=u"('div.popup-build-informational')[0]",
+ validator=u'1 new recipe build has been queued.')
+
# And check that there's an error.
client.waits.forElement(
jquery=u"('div.yui3-lazr-formoverlay-errors ul li')",
Follow ups
