Bläddra i källkod

Report post saves through the jsalert banner

The plumbing for this was already half-built -- posts() passes failure and
message, and posts.tx includes jsalert.tx -- but post_save answered with a bare
303 that carried no state, so failure always defaulted to -1 and the banner
stayed hidden. A rejected post got a dead-end 400 instead.

post_save now redirects to where you came from carrying the outcome, and
posts()/index() turn that back into the failure and message the banner reads.
Redirecting rather than rendering in place keeps a save as a
POST-redirect-GET, so refreshing the page you land on cannot re-submit the
post; that means the outcome has to travel in the URL. The full validator
output still goes to the log, and a one-line version goes in the banner.

Two bugs in jsalert.tx itself, both of which had to be fixed for any of this to
show:

It computed `$failure ? $failure : -1`. Success is 0, which is falsy, so every
success collapsed to -1 and the banner hid itself. That means the success case
has never once displayed -- including "Login Successful, Redirecting...", whose
redirect lives in that same branch.

Its success branch always redirected, so a banner could only ever be shown on
the way somewhere else. It is now conditional on $to, which lets a page report
an outcome and stay put. Callers that do set $to -- login, the post type wizard
-- are unaffected.

posts.tx gated the banner on $to for the same reason; it now shows it whenever
there is something to say.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U3k2hhSZpcsoe6uBmhF9nB
George Baugh 2 månader sedan
förälder
incheckning
cb9e886486

+ 59 - 3
lib/Trog/Routes/HTML.pm

@@ -271,6 +271,53 @@ if ($theme_dir) {
 
 =head1 PRIMARY ROUTE
 
+=head2 _feedback_redirect($query, $to, $failure, $message)
+
+Redirect to $to, carrying the outcome of a save so the destination page can put
+it in the jsalert banner.
+
+We redirect rather than rendering in place so that a save stays a
+POST-redirect-GET -- refreshing the page the user lands on must not re-submit
+the post.  That means the outcome has to travel in the URL, as there is nowhere
+else to put it.
+
+=cut
+
+sub _feedback_redirect ( $query, $to, $failure, $message ) {
+
+    # The banner puts this in a JS string literal, so it has to be one line.
+    $message //= '';
+    $message =~ s/\s*\n+\s*/; /g;
+    $message =~ s/;\s*$//;
+
+    my $sep = $to =~ m/\?/ ? '&' : '?';
+    my $key = $failure     ? 'savefailed' : 'saved';
+    return $query->{tpsgi}->see_also( $to . $sep . $key . '=' . URI::Escape::uri_escape($message) );
+}
+
+=head2 _absorb_feedback($query)
+
+Turn the parameters _feedback_redirect() put in the URL back into the failure
+and message the jsalert banner reads.
+
+Done in index() rather than in each route, so that a save can hand feedback to
+any destination that renders a page, whichever route ends up serving it.
+
+=cut
+
+sub _absorb_feedback ($query) {
+    my ( $saved, $failed ) = ( $query->{saved}, $query->{savefailed} );
+    return unless defined $saved || defined $failed;
+
+    $query->{failure} = defined $failed ? 1 : 0;
+    $query->{message} = ( defined $failed ? $failed : $saved ) || ( $query->{failure} ? 'Save failed.' : 'Saved.' );
+
+    # Nothing further to navigate to -- we are already there.  Without this the
+    # banner's success branch would bounce us onwards.
+    delete $query->{to};
+    return;
+}
+
 =head2 index
 
 Implements the primary route used by all pages not behind auth.
@@ -281,6 +328,8 @@ Most subsequent functions simply pass content to this function.
 sub index ( $query, $content = '', $i_styles = [], $i_scripts = [] ) {
     $query->{theme_dir} = Trog::Themes::td();
 
+    _absorb_feedback($query);
+
     my $to_render = $query->{template} // $landing_page;
 
     $content ||= Trog::Renderer->render( template => $to_render, data => $query, component => 1, contenttype => 'text/html' );
@@ -893,8 +942,11 @@ sub post_save ($qq) {
     eval { $data->add($query); 1 } or do {
         my @errors = Ref::Util::is_arrayref($@) ? @{$@} : ($@);
         my $why    = "Post failed validation:\n" . join( "\n", @errors );
+
+        # The full detail goes to the log; the user gets it in the banner on
+        # the page they came from, rather than a dead-end 400.
         WARN("Rejected post from $qq->{user}: $why");
-        return $qq->{tpsgi}->badrequest( $qq, $why );
+        return _feedback_redirect( $qq, '/secure' . $to, 1, $why );
     };
 
     # Instruct tpsgi to invalidate the cached render.
@@ -907,7 +959,7 @@ sub post_save ($qq) {
     # Force a reload of the routing table
     $qq->{tpsgi}->signal_restart_parent();
 
-    return $qq->{tpsgi}->see_also('/secure'.$to);
+    return _feedback_redirect( $qq, '/secure' . $to, 0, "Saved post '$query->{title}'." );
 }
 
 =head2 profile
@@ -1171,6 +1223,10 @@ Display multi or single posts, supports RSS and pagination.
 
 sub posts ( $query, $direct = 0 ) {
 
+    # Before we build the render data, not after: posts.tx is rendered here and
+    # handed to index() as content, so index()'s own absorb would be too late.
+    _absorb_feedback($query);
+
     # Allow rss.xml to tell what posts to loop over
     my $fmt = $query->{format} || '';
 
@@ -1359,7 +1415,7 @@ sub posts ( $query, $direct = 0 ) {
             post_visibilities => \@visibuddies,
             failure           => $query->{failure},
             to                => $query->{to},
-            message           => $query->{failure} ? "Failed to add post!" : "Successfully added Post as $query->{id}",
+            message           => $query->{message} // ( $query->{failure} ? "Failed to add post!" : "Successfully added Post as $query->{id}" ),
             direct            => $direct,
             title             => $query->{title},
             author            => $query->{primary_post}{user} // $posts[0]{user},

+ 79 - 3
t/Trog-Routes-HTML-series-lifecycle.t

@@ -25,6 +25,7 @@ use File::Find     ();
 use File::Path     ();
 use File::Temp     ();
 use Time::HiRes    ();
+use URI::Escape    ();
 
 our ( $REPO, $ROOT, $OLDCWD );
 our %REPO_BEFORE;
@@ -305,10 +306,15 @@ sub _save_post {
         return undef;
     }
 
-    my ( $code, undef, $body ) = @$res;
-    if ( $code != 303 ) {
+    my ( $code, $headers, $body ) = @$res;
+    my %h = @{ $headers || [] };
+
+    # Both outcomes redirect now, so the status alone proves nothing -- the
+    # outcome is in the query string post_save hands to the destination.
+    if ( $code != 303 || ( $h{Location} // '' ) =~ m/savefailed=/ ) {
         fail("$label: saved");
-        diag( "post_save returned $code:\n" . join( '', @{ $body || [''] } ) );
+        diag( "post_save returned $code, Location: " . ( $h{Location} // '(none)' ) );
+        diag( join( '', @{ $body || [''] } ) ) if $body && @$body;
         return undef;
     }
     pass("$label: saved");
@@ -683,6 +689,76 @@ subtest 'spec_widget (the wizard-built type)' => sub {
     like( $body, qr/Spec widget body\./, 'and the body came through the generated template' );
 };
 
+subtest 'saves report back through the jsalert banner' => sub {
+
+    # A good save redirects to where you came from, carrying the outcome.
+    my $res = Trog::Routes::HTML::post_save(
+        _admin(
+            route      => '/post/save',
+            method     => 'POST',
+            to         => '/specblog',
+            form       => 'blog.tx',
+            title      => 'Spec Feedback Child',
+            data       => 'Spec feedback body.',
+            visibility => 'public',
+            tags       => ['specblog'],
+        )
+    );
+    my ( $code, $headers ) = @$res;
+    my %h = @{ $headers || [] };
+    is( $code, 303, 'a good save redirects' );
+    like( $h{Location}, qr{^/secure/specblog\?saved=}, 'to where we came from, flagged as saved' );
+    unlike( $h{Location}, qr/\n/, 'and the message is one line, since it lands in a JS string' );
+
+    _reindex();
+
+    # Follow the redirect the way a browser would.
+    my ($qs) = $h{Location} =~ m/\?(.*)$/;
+    my %params = map { my ( $k, $v ) = split( /=/, $_, 2 ); ( $k => URI::Escape::uri_unescape($v) ) } split( /&/, $qs );
+
+    my ( $rcode, $body, $err ) = _render(
+        _admin( route => '/specblog', has_query => 1, %params ),
+        \&Trog::Routes::HTML::series,
+    );
+    is( $rcode, 200, 'the destination renders' ) or diag($err);
+    like( $body, qr/var loginFailure = 0;/,               'the banner is in success mode' );
+    like( $body, qr/Saved post &#39;Spec Feedback Child&#39;\./, 'and says what was saved' );
+
+    # Nothing to navigate on to, so the banner must not bounce us anywhere.
+    unlike( $body, qr/window\.location=/, 'and does not redirect onwards' );
+
+    # A rejected save comes back the same way, in red.
+    $res = Trog::Routes::HTML::post_save(
+        _admin(
+            route      => '/post/save',
+            method     => 'POST',
+            to         => '/specblog',
+            form       => 'blog.tx',
+            title      => 'Spec Rejected Child',
+            visibility => 'not-a-visibility',
+            tags       => ['specblog'],
+        )
+    );
+    ( $code, $headers ) = @$res;
+    %h = @{ $headers || [] };
+    is( $code, 303, 'a rejected save redirects too, rather than dead-ending on a 400' );
+    like( $h{Location}, qr{^/secure/specblog\?savefailed=}, 'flagged as failed' );
+    like( URI::Escape::uri_unescape( $h{Location} ), qr/Not in enum list/, 'carrying the validator error' );
+
+    ok( !_find_post('Spec Rejected Child'), 'and the bad post was not written' );
+
+    ($qs) = $h{Location} =~ m/\?(.*)$/;
+    %params = map { my ( $k, $v ) = split( /=/, $_, 2 ); ( $k => URI::Escape::uri_unescape($v) ) } split( /&/, $qs );
+
+    ( $rcode, $body, $err ) = _render(
+        _admin( route => '/specblog', has_query => 1, %params ),
+        \&Trog::Routes::HTML::series,
+    );
+    is( $rcode, 200, 'the destination still renders' ) or diag($err);
+    like( $body, qr/var loginFailure = 1;/, 'the banner is in failure mode' );
+    like( $body, qr/Not in enum list/,      'and shows why' );
+};
+
 subtest 'the topbar did not silently overflow' => sub {
     my ( $code, $body, $err ) = _render( _anon( route => '/' ) );
     is( $code, 200, '/ renders' ) or diag($err);

+ 5 - 1
www/templates/html/components/jsalert.tx

@@ -1,6 +1,8 @@
 <script>
     document.addEventListener("DOMContentLoaded", function(event) {
-        var loginFailure = <: $failure ? $failure : -1 :>;
+        // Defined-or, not a ternary: success is 0, which is falsy, so the old
+        // test collapsed every success to -1 and hid the banner entirely.
+        var loginFailure = <: $failure // -1 :>;
         if( loginFailure === -1 ) {
             document.querySelector('#jsalert').style.cssText = 'display:none;';
         } else if ( loginFailure === 1 ) {
@@ -13,10 +15,12 @@
             document.querySelector('#jsalert').classList.add("alert-success");
             document.querySelector('#msgIcon').innerHTML = "✓";
             document.querySelector('#message').innerHTML = "<: $message :>";
+            : if ($to) {
             //JS redir better ux than 302
             window.setTimeout(function() {
                 window.location="<: $to :>";
             }, 500);
+            : }
         }
     });
 </script>

+ 1 - 1
www/templates/html/components/posts.tx

@@ -4,7 +4,7 @@
     <: $emoji_picker | mark_raw :>
     </div>
     : if (!$direct) {
-        : if ($to) {
+        : if ($failure != -1) {
             : include "jsalert.tx";
         : }
         <script type="text/javascript">