Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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]
  • Loading branch information...
commit 9c802d12950b2d4bb42b58aadf24e36db67c6fee 1 parent ca5f60f
@theory theory authored
View
58 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,7 +79,7 @@ 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') {
@@ -86,6 +87,7 @@ sub checkin : Callback {
$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,6 +214,7 @@ sub move : Callback {
}
sub publish : Callback {
+ my ($self, $allow_fatal) = @_;
my $self = shift;
my $param = $self->params;
my $story_pub = $param->{story_pub} || {};
@@ -399,11 +403,24 @@ 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;# || !isa_bric_exception $err, 'Error';
+ $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 {
$self->set_redirect('/workflow/profile/publish');
}
@@ -648,10 +665,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);
@@ -675,7 +696,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
46 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,6 +190,7 @@ sub checkin : Callback(priority => 6) {
asset => $story });
$cur_desk->save;
} else {
+ $cur_desk = $pub_desk;
$pub_desk->accept({ asset => $story });
}
$pub_desk->save;
@@ -195,18 +198,6 @@ sub checkin : Callback(priority => 6) {
$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,32 @@ 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() });
+ $story->set_workflow_id($work_id);
+ $cur_desk->accept({ asset => $story });
+ $cur_desk->save;
+ $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;
}
Please sign in to comment.
Something went wrong with that request. Please try again.