Skip to content

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also .

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also .
...
  • 6 commits
  • 6 files changed
  • 0 commit comments
  • 1 contributor
Commits on Aug 16, 2011
@theory theory Fix handling of publish failures in Ajax publish requests.
This fixes the half of bug #277, when one attempts to publish from a desk or workspace.

When `PUBLISH_RELATED_FAIL_BEHAVIOR = fail` and a related asset fails to publish, we now call `raise_conflict` to return a 409 and do a better job displaying *all* the appropriate error messages.

When `PUBLISH_RELATED_FAIL_BEHAVIOR = warn` and  related asset fails to publish, we call a new moethod, `show_accepted()`. This method returns a 202 status code, which I'm abusing a bit here, but it comes closest to what we want. The story properly publishes and disappears from the desk, but a new handler in the Ajax code also shows the errors when related failed to publish. I also added code to abort at the end of the Desk `publish` callback when a request is Ajax so that no other stuff gets sent back to the browser. This is because in "warn" mode, we wnt the full request to succeed, with no rollbacks or anything, and all subsequent code should execute, so that the story will properly be published and removed from workflow.

Tomorrow I'll have to figure out what to do about non-ajax publish requests in order to properly and finally fix bug # 277.
cde51fd
Commits on Aug 17, 2011
@theory theory Move fixes for Bug #277.
With `PUBLISH_RELATED_FAIL_BEHAVIOR = fail`, things work fine now when publishing via "Check in and Publish" in the story profile and when selecting that option on a desk or workflow, which is an Ajax call.

One side effect is that if the publish fails because the story itself fails to publish (rather than because a related fails), the story is instantly checked out to the user again and put back into workflow. In the interrim, the asset will have been checked in, so the result is a new version number. I think that this is a very minor issue that most folks won't even notice, and is far better than what we had, where things were checked in and sometimes removed from workflow. This is better: other than the new version, it looks like the same page as before, with all the same data, but a nice status message explaining the failure.

In order to properly catch an error when there are no destinations, that exception is now thorwn as an "invalid error" exception rather than a fatal exception. That indicates that it's something to inform the user of, rather than something unexpected (which is a 500). Looking at it, I think it was silly to have been throwing a burn error for that particular error; an invalid error is a much better choice.

Still to do to finish fixing this bug:

* Fix media to follow the same pattern.
* Make sure that `PUBLISH_RELATED_FAIL_BEHAVIOR = warn` works as expected
* Make sure that publishing from search results works as expected
* Make sure that bulk publish works as expected.
* Make sure all tests continue to pass.

[#277 state:open]
e37ac2e
@theory theory Make media publish work the same as story publish.
More work on [#277 state:open].
428dbf8
@theory theory Yet more fixes for Bug #277.
Still with `PUBLISH_RELATED_FAIL_BEHAVIOR = fail`, make sure publishes that fail because realteds fail to publish works properly both from desks and from document profiles.

While at it, I filled in some other gaps in the publish logging and rollback logic in the Desk callback.

[#277 state:open]
642052e
@theory theory More Bug # 277 fixes.
This time, always die on non-ajax requests. This allows the "publish later" option on the Publish desk to throw an error and return to the same page, rather than load the scheduling page and show the error there.
4ee39a0
@theory theory Fold $is_ajax into $allow_fail to simplify things. 7bba7a7
View
6 comp/media/js/lib.js
@@ -1239,7 +1239,11 @@ var Desk = {
onSuccess: onSuccess,
onFailure: Bricolage.handleError,
on403: Bricolage.handleForbidden,
- on409: Bricolage.handleConflict
+ on409: Bricolage.handleConflict,
+ on202: function(req) {
+ Bricolage.handleConflict(req);
+ onSuccess(req);
+ }
} );
},
View
47 lib/Bric/App/Callback.pm
@@ -6,8 +6,8 @@ __PACKAGE__->register_subclass;
use constant CLASS_KEY => 'Callback';
use HTML::Entities;
use Bric::App::Cache;
-use Bric::App::Util qw(get_pref add_msg);
-use Bric::Util::ApacheConst qw(HTTP_CONFLICT HTTP_FORBIDDEN);
+use Bric::App::Util qw(get_pref add_msg get_msg clear_msg);
+use Bric::Util::ApacheConst qw(HTTP_CONFLICT HTTP_FORBIDDEN HTTP_ACCEPTED);
use Bric::Util::DBI qw(begin rollback);
use Bric::Util::Language;
@@ -24,34 +24,49 @@ sub add_message {
sub raise_status {
my ($self, $status) = (shift, shift);
- my $r = $self->apache_req or return $self->add_message(@_);
+ $self->add_message(@_);
+ my $r = $self->apache_req or return;
# If it's not an Ajax request, it's just a message.
- return $self->add_message(@_)
- if ($r->headers_in->{'X-Requested-With'} || '') ne 'XMLHttpRequest';
+ return if ($r->headers_in->{'X-Requested-With'} || '') ne 'XMLHttpRequest';
# Abort the database transaction.
rollback(1);
begin(1);
- # Prep the error message.
- my $txt = Bric::Util::Language->instance->maketext(@_);
- if ($txt =~ /(.*)<span class="l10n">(.*)<\/span>(.*)/) {
- $txt = encode_entities($1) . '<span class="l10n">'
- . encode_entities($2) . '</span>' . encode_entities($3);
- } else {
- $txt = encode_entities($txt);
- }
-
# Send the status to the browser and abort.
- $r->status($status);
- $r->print(qq{<div class="errorMsg">$txt</div>});
+ $self->_print_messages($r, $status);
$self->abort;
}
sub raise_conflict { shift->raise_status( HTTP_CONFLICT, @_ ) }
sub raise_forbidden { shift->raise_status( HTTP_FORBIDDEN, @_ ) }
+sub show_accepted {
+ my $self = shift;
+ $self->add_message(@_);
+ my $r = $self->apache_req or return;
+
+ # If it's not an Ajax request, it's just a message.
+ return if ($r->headers_in->{'X-Requested-With'} || '') ne 'XMLHttpRequest';
+
+ $self->_print_messages($r, HTTP_ACCEPTED);
+}
+
+sub _print_messages {
+ my ($self, $r, $status) = @_;
+ # Prep the error message(s).
+ my $txt = join '<br /><br />', map {
+ /(.*)<span class="l10n">(.*)<\/span>(.*)/
+ ? encode_entities($1) . '<span class="l10n">'
+ . encode_entities($2) . '</span>' . encode_entities($3)
+ : encode_entities($_)
+ } get_msg;
+ clear_msg;
+ $r->status($status);
+ $r->print(qq{<div class="errorMsg">$txt</div>});
+}
+
1;
=head1 Name
View
99 lib/Bric/App/Callback/Desk.pm
@@ -69,6 +69,7 @@ sub checkin : Callback {
return;
}
+ my $checked_out_to = $obj->get_user_id;
$desk->checkin($obj);
log_event("${class}_checkin", $obj, { Version => $obj->get_version });
@@ -78,14 +79,15 @@ sub checkin : Callback {
$obj->save;
log_event("${class}_rem_workflow", $obj);
} elsif ($next_desk_id eq 'publish') {
- $self->_move_to_publish_desk($obj);
+ $self->_move_to_publish_desk($obj, $checked_out_to);
$self->params->{"${class}_pub"} = { $obj->get_version_id => $obj };
$self->publish;
} elsif ($next_desk_id eq 'deploy') {
if ($class eq 'template') {
$self->_move_to_publish_desk($obj);
$self->params->{"desk_asset|template_pub_ids"} = [ $obj->get_version_id ];
$self->deploy;
+ $self->_log_desk_move;
}
} elsif ($next_desk_id) {
if ($desk->get_id != $next_desk_id) {
@@ -180,6 +182,7 @@ sub move : Callback {
$self->_move_to_publish_desk($obj);
$self->params->{"desk_asset|template_pub_ids"} = [ $obj->get_version_id ];
$self->deploy;
+ $self->_log_desk_move;
}
return;
}
@@ -211,10 +214,16 @@ sub move : Callback {
}
sub publish : Callback {
+ my ($self, $allow_fatal) = @_;
my $self = shift;
my $param = $self->params;
my $story_pub = $param->{story_pub} || {};
my $media_pub = $param->{media_pub} || {};
+ my $is_ajax = do {
+ my $r = $self->apache_req;
+ $r && ($r->headers_in->{'X-Requested-With'} || '') eq 'XMLHttpRequest';
+ };
+ $allow_fatal ||= !$is_ajax;
# If we were passed a string instead of an object, find the object
for my $pub (\$story_pub, \$media_pub) {
@@ -366,19 +375,32 @@ sub publish : Callback {
# By this point we now know if we're going to fail this publish
# if we set the fail behaviour to fail rather than warn
if (PUBLISH_RELATED_ASSETS && @messages) {
+ $self->add_message(@$_) for @messages;
if (PUBLISH_RELATED_FAIL_BEHAVIOR eq 'fail') {
- $self->add_message(@$_) for @messages;
- my $msg = 'Publish aborted due to errors above. Please fix the '
- . ' above problems and try again.';
- throw_error error => $msg,
- maketext => $msg;
+ # If it was on a desk, we need to revert its workflow status.
+ $self->_revert_to_original_state;
+ my $err = 'Publish aborted due to errors above. Please fix the '
+ . 'above problems and try again.';
+ # Throw an error or a conflict as appropriate.
+ throw_error(
+ error => $err,
+ maketext => [$err]
+ ) if $allow_fatal;
+ $self->raise_conflict([$err]);
} else {
# we are set to warn, should we add a further warning to the msg ?
- $self->raise_conflict(@$_) for @messages,
- 'Some of the related assets were not published.';
+ $self->show_accepted(
+ ['Some of the related assets were not published.']
+ );
}
- } else {
- $self->raise_conflict(@$_) for @messages;
+ } elsif (@messages) {
+ # If it was on a desk, we need to revert its workflow status.
+ $self->_revert_to_original_state;
+
+ # Add all messages and raise a conflict.
+ my $last = pop @messages;
+ $self->add_message(@$_) for @messages;
+ $self->raise_conflict(@$last);
}
# For publishing from a desk, I added two new 'publish'
@@ -400,14 +422,32 @@ sub publish : Callback {
cb_request => $self->cb_request,
pkg_key => 'publish',
apache_req => $self->apache_req,
- params => { instant => 1,
- pub_date => strfdate(),
- },
+ params => {
+ instant => 1,
+ pub_date => strfdate(),
+ },
);
- $pub->publish();
+ eval { $pub->publish };
+ if (my $err = $@) {
+ # If it was on a desk, we need to revert its workflow status.
+ $self->_revert_to_original_state;
+ # Continue with normal error handling.
+ die $err if $allow_fatal;
+ $self->raise_conflict($err->maketext);
+ } else {
+ # Success! Need to log the move to a desk. Yes, this is late, but
+ # better than the alternative, which is leaving the log message
+ # there in the case of publish failure.
+ $self->_log_desk_move;
+ }
} else {
+ # Just leave it to the queue.
+ $self->_log_desk_move;
$self->set_redirect('/workflow/profile/publish');
}
+
+ # Don't render anything for Ajax requests.
+ $self->abort if $is_ajax;
}
sub deploy : Callback {
@@ -645,10 +685,14 @@ sub _merge_properties {
}
sub _move_to_publish_desk {
- my ($self, $obj) = @_;
+ my ($self, $obj, $user_id) = @_;
my $class = $obj->key_name;
- my $cur_desk = $obj->get_current_desk;
+ my $cur_desk = $self->{pub_from_desk} = $obj->get_current_desk;
+ $self->{pub_from_work_id} = $obj->get_workflow_id;
+ $self->{pub_obj} = $obj;
+ $self->{pub_by_user_id} = $user_id;
+ print STDERR "Pub by $user_id\n";
# Publish the template and remove it from workflow.
my ($pub_desk, $no_log);
@@ -672,7 +716,28 @@ sub _move_to_publish_desk {
$obj->save;
- log_event("${class}_moved", $obj, { Desk => $pub_desk->get_name }) unless $no_log;
+ $self->{log_desk_move} = sub {
+ log_event("${class}_moved", $obj, { Desk => $pub_desk->get_name })
+ } unless $no_log;
+}
+
+sub _revert_to_original_state {
+ my $self = shift;
+ my $obj = delete $self->{pub_obj} or return;
+ my $work_id = delete $self->{pub_from_work_id} or return;
+ $obj->checkout({ user__id => delete $self->{pub_by_user_id} })
+ if defined $self->{pub_by_user_id};
+ $obj->set_workflow_id($work_id);
+ my $desk = delete $self->{pub_from_desk};
+ $desk->accept({ asset => $obj });
+ $desk->save;
+ $obj->save;
+}
+
+sub _log_desk_move {
+ my $self = shift;
+ my $code = delete $self->{log_desk_move} or return;
+ $code->();
}
1;
View
56 lib/Bric/App/Callback/Profile/Media.pm
@@ -257,6 +257,8 @@ sub checkin : Callback(priority => 6) {
{ Workflow => $wf->get_name });
$media->checkout({ user__id => get_user_id() })
unless $media->get_checked_out;
+ } else {
+ $work_id = $media->get_workflow_id;
}
$media->checkin;
@@ -300,18 +302,6 @@ sub checkin : Callback(priority => 6) {
$media->save;
- # Log it!
- log_event('media_save', $media);
- log_event('media_checkin', $media, { Version => $media->get_version });
- my $dname = $pub_desk->get_name;
- log_event('media_moved', $media, { Desk => $dname })
- unless $no_log;
- $self->add_message(
- 'Media "[_1]" saved and checked in to "[_2]".',
- $media->get_title,
- $dname
- );
-
# Prevent loss of data due to publish failure.
commit(1);
begin(1);
@@ -324,16 +314,42 @@ sub checkin : Callback(priority => 6) {
params => { media_pub => { $media->get_version_id => $media } },
);
- # Clear the state out, set redirect, and publish.
- $self->clear_my_state;
- $pub->publish;
- if (my $prev = get_state_data('_profile_return')) {
- $self->return_to_other($prev);
+ # Make it so!
+ eval { $pub->publish(1) };
+ if (my $err = $@) {
+ # FAIL! Put the story back into workflow and act as if nothing
+ # ever happened to its workflow status.
+ $media->checkout({ user__id => get_user_id() })
+ unless $media->get_checked_out;
+ $media->set_workflow_id($work_id) unless defined $media->get_workflow_id;
+ $cur_desk->accept({ asset => $media });
+ $cur_desk->save;
+ # Need to commit in order to make it stick!
+ commit(1);
+ begin(1);
+ $media->save;
+ die $err;
} else {
- $self->set_redirect('/');
- }
-
+ # Log it, clear the state out, and set redirect.
+ $self->clear_my_state;
+ $pub->publish;
+ if (my $prev = get_state_data('_profile_return')) {
+ $self->return_to_other($prev);
+ } else {
+ $self->set_redirect('/');
+ }
+ log_event('media_save', $media);
+ log_event('media_checkin', $media, { Version => $media->get_version });
+ my $dname = $pub_desk->get_name;
+ log_event('media_moved', $media, { Desk => $dname })
+ unless $no_log;
+ $self->add_message(
+ 'Media "[_1]" saved and checked in to "[_2]".',
+ $media->get_title,
+ $dname
+ );
+ }
} else {
# Look up the selected desk.
my $desk = Bric::Biz::Workflow::Parts::Desk->lookup
View
50 lib/Bric/App/Callback/Profile/Story.pm
@@ -147,6 +147,8 @@ sub checkin : Callback(priority => 6) {
{ Workflow => $wf->get_name });
$story->checkout({ user__id => get_user_id() })
unless $story->get_checked_out;
+ } else {
+ $work_id = $story->get_workflow_id;
}
$story->checkin;
@@ -188,25 +190,14 @@ sub checkin : Callback(priority => 6) {
asset => $story });
$cur_desk->save;
} else {
+ $cur_desk = $pub_desk;
$pub_desk->accept({ asset => $story });
}
$pub_desk->save;
}
$story->save;
- # Log it!
- log_event('story_save', $story);
- log_event('story_checkin', $story, { Version => $story->get_version });
- my $dname = $pub_desk->get_name;
- log_event('story_moved', $story, { Desk => $dname })
- unless $no_log;
- $self->add_message(
- 'Story "[_1]" saved and checked in to "[_2]".',
- '<span class="l10n">' . $story->get_title . '</span>',
- $dname,
- );
-
# Prevent loss of data due to publish failure.
commit(1);
begin(1);
@@ -218,11 +209,36 @@ sub checkin : Callback(priority => 6) {
params => { story_pub => { $story->get_version_id => $story } },
);
- # Clear the state out, set redirect, and publish.
- $self->clear_my_state;
- $self->set_redirect('/');
- $pub->publish;
-
+ # Make it so!
+ eval { $pub->publish(1) };
+ if (my $err = $@) {
+ # FAIL! Put the story back into workflow and act as if nothing
+ # ever happened to its workflow status.
+ $story->checkout({ user__id => get_user_id() })
+ unless $story->get_checked_out;
+ $story->set_workflow_id($work_id) unless defined $story->get_workflow_id;
+ $cur_desk->accept({ asset => $story });
+ $cur_desk->save;
+ # Need to commit in order to make it stick!
+ commit(1);
+ begin(1);
+ $story->save;
+ die $err;
+ } else {
+ # Log it, clear the state out, and set redirect.
+ $self->clear_my_state;
+ $self->set_redirect('/');
+ log_event('story_save', $story);
+ log_event('story_checkin', $story, { Version => $story->get_version });
+ my $dname = $pub_desk->get_name;
+ log_event('story_moved', $story, { Desk => $dname })
+ unless $no_log;
+ $self->add_message(
+ 'Story "[_1]" saved and checked in to "[_2]".',
+ '<span class="l10n">' . $story->get_title . '</span>',
+ $dname,
+ );
+ }
} else {
# Look up the selected desk.
my $desk = Bric::Biz::Workflow::Parts::Desk->lookup
View
16 lib/Bric/Util/Burner.pm
@@ -211,7 +211,7 @@ use strict;
# Programatic Dependencies
use Bric::Util::Fault qw(throw_gen throw_burn_error throw_burn_user
- rethrow_exception);
+ rethrow_exception throw_invalid);
use Bric::Util::Trans::FS;
use Bric::Config qw(:burn :mason :time PREVIEW_LOCAL ENABLE_DIST :prev :l10n);
use Bric::Biz::OutputChannel qw(:burners);
@@ -1299,12 +1299,14 @@ sub publish {
my $errstr = q{Cannot publish asset "} . $ba->get_name
. q{" to "} . $oc->get_name . q{" because there }
. "are no Destinations associated with this output channel.";
- throw_burn_error error => $errstr,
- mode => $self->get_mode,
- oc => $oc->get_name,
- elem => $at->get_name,
- element => $at
- if $die_err;
+ throw_invalid(
+ error => $errstr,
+ maketext => [
+ 'Cannot publish asset "[_1]" to "[_2]" because there are no Destinations associated with this output channel.',
+ $ba->get_name,
+ $oc->get_name
+ ]
+ ) if $die_err;
add_msg($errstr);
next;
}

No commit comments for this range

Something went wrong with that request. Please try again.