Преглед изворни кода

Read the rest of the wizard's checkboxes off the template too

The canned blocks that go into a type's editor were fixed by reading its
': include' lines; the ones that do not go into the editor were left reading the
sidecar, and nothing but a generated sidecar records those.  So every one of them
came back clear for every type on disk, including the generated ones -- guests
and provisioned_guests predate two of the three being recorded at all.

Reopening blog listed the wrapper, the post title and the post tag list as
switched off while its template includes all three.  Same reading as the
includes, then, and for the same reason: the template is what renders, so it is
the template that gets asked, with the sidecar left as the fallback for a type
whose template cannot be read at all.

The body form is the same bug wearing a different hat.  It is an include like
any other -- _wizard_template emits it as one -- but it was read out of the
sidecar, so presentation and invoice both reopened as form_common.tx.  Saving
either of them back would have quietly turned a multi-page type into a single
page one, which is the sort of thing you find out about later.

And a browser test, since none of this is visible from below the browser: the
wizard is server data in a script element applied by client JS, and a unit test
can prove the JSON is right and still tell you nothing about what an admin sees.
It drives the real thing with Playwright, picks each of four stock types, and
asserts every checkbox, the body form and the custom field rows.  It needs a
running server and an admin to log in as, and skips without them.

It logs in by posting to /auth rather than typing into the login page: on a
successful login jsalert.tx schedules a window.location half a second after
DOMContentLoaded, and the login form navigates out from under anything still
filling it in.  The session lands in the same cookie jar either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RprkeFWfYwKF9ZsgND8om
George Baugh пре 1 месец
родитељ
комит
4c62609309
3 измењених фајлова са 262 додато и 10 уклоњено
  1. 64 10
      lib/Trog/Routes/HTML.pm
  2. 37 0
      t/Trog-DataModule-schema.t
  3. 161 0
      t/Trog-Routes-HTML-post-wizard-browser.t

+ 64 - 10
lib/Trog/Routes/HTML.pm

@@ -2246,7 +2246,13 @@ our @wizard_optional_includes = grep {
     !grep { $include eq $_ } @wizard_mandatory
     !grep { $include eq $_ } @wizard_mandatory
 } @wizard_include_order;
 } @wizard_include_order;
 
 
-our @wizard_checkboxes         = ( qw{wrapper inc_post_title inc_post_tags}, @wizard_optional_includes, 'overwrite' );
+# The checkboxes which are not editor includes: the wrapper div, and the two
+# canned blocks a type splices into its display half.  Read back out of the
+# template by _wizard_display_of(), since %wizard_includes covers only the
+# blocks that go into the editor.
+our @wizard_display_checkboxes = qw{wrapper inc_post_title inc_post_tags};
+
+our @wizard_checkboxes         = ( @wizard_display_checkboxes, @wizard_optional_includes, 'overwrite' );
 our %wizard_checked_by_default = map { $_ => 1 } qw{wrapper inc_post_title inc_post_tags inc_preview inc_tags inc_aliases};
 our %wizard_checked_by_default = map { $_ => 1 } qw{wrapper inc_post_title inc_post_tags inc_preview inc_tags inc_aliases};
 
 
 # Which HTML input the wizard emits, and what that field is in the sidecar's
 # Which HTML input the wizard emits, and what that field is in the sidecar's
@@ -2677,21 +2683,24 @@ sub _wizard_types ($forms) {
         my $display = $type->{display};
         my $display = $type->{display};
         $display = _wizard_display_from_template($form) unless length( $display // '' );
         $display = _wizard_display_from_template($form) unless length( $display // '' );
 
 
+        # The wrapper div and the two blocks which belong to a type's display
+        # half rather than to its editor.  Same reading as the includes, and for
+        # the same reason: a hand-written type recorded none of it, and the
+        # generated ones written before these were recorded did not either.
+        my $shown = _wizard_display_of($form) // { map { $_ => $type->{$_} ? 1 : 0 } @wizard_display_checkboxes };
+
         $types{$form} = {
         $types{$form} = {
             name              => ( $form =~ s/\.tx$//r ),
             name              => ( $form =~ s/\.tx$//r ),
-            description       => $type->{description}         // '',
-            display           => $display                     // '',
-            body_form         => $type->{body_form}           // 'form_common.tx',
+            description       => $type->{description} // '',
+            display           => $display             // '',
+            body_form         => _wizard_body_form_of( \%includes, $type ),
             datasource        => $meta->{'x-tcms-datasource'} // '',
             datasource        => $meta->{'x-tcms-datasource'} // '',
             title_placeholder => $type->{title_placeholder}   // '',
             title_placeholder => $type->{title_placeholder}   // '',
             generated         => $type->{generated} ? 1 : 0,
             generated         => $type->{generated} ? 1 : 0,
 
 
-            # The wizard's own checkboxes.  The includes are a list in the
-            # sidecar; the other three are recorded individually because nothing
-            # else would say whether they were ticked.
-            wrapper        => $type->{wrapper}        ? 1 : 0,
-            inc_post_title => $type->{inc_post_title} ? 1 : 0,
-            inc_post_tags  => $type->{inc_post_tags}  ? 1 : 0,
+            # The wizard's own checkboxes, every one of them an answer about
+            # what the template does rather than about what the sidecar says.
+            %$shown,
             ( map { $_ => $includes{ $wizard_includes{$_} } ? 1 : 0 } @wizard_optional_includes ),
             ( map { $_ => $includes{ $wizard_includes{$_} } ? 1 : 0 } @wizard_optional_includes ),
 
 
             fields => _wizard_fields_of( $schema, $meta, \%includes ),
             fields => _wizard_fields_of( $schema, $meta, \%includes ),
@@ -2701,6 +2710,51 @@ sub _wizard_types ($forms) {
     return \%types;
     return \%types;
 }
 }
 
 
+# Which body form a type carries, which is an include like any other -- see the
+# bottom of _wizard_template.  The sidecar is the fallback for a type whose
+# template says nothing, which is what a datasource type with no editor at all
+# looks like.
+sub _wizard_body_form_of ( $includes, $type ) {
+    return 'form_multi.tx'  if $includes->{'form_multi.tx'};
+    return 'form_common.tx' if $includes->{'form_common.tx'};
+    return $type->{body_form} // 'form_common.tx';
+}
+
+# What a type's template says about the parts of it the wizard emits itself
+# rather than through %wizard_includes: the wrapper div, and the two canned
+# blocks which go in the display half rather than in the editor.
+#
+# Undef when the template cannot be read at all, which is what leaves the
+# sidecar as the fallback rather than making it a second opinion.  The template
+# is what actually renders, so where the two disagree it is the template which
+# is telling the truth.
+sub _wizard_display_of ($form) {
+    my $path = Trog::Themes::themed_file_in_dir( 'forms', $form, 'text/html', 1 );
+    return undef unless $path;
+
+    my $template = eval { File::Slurper::read_text($path) };
+    return undef unless defined $template;
+
+    my %shown = (
+        inc_post_title => $template =~ m/^\s*:\s*include\s+"post_title\.tx"/m ? 1 : 0,
+        inc_post_tags  => $template =~ m/^\s*:\s*include\s+"post_tags\.tx"/m  ? 1 : 0,
+        wrapper        => 0,
+    );
+
+    # The wrapper is the first thing a type that has one emits, ahead of any
+    # Kolon at all -- see _wizard_template.  So only the first line with
+    # anything on it is asked, and a type which opens with its own markup or its
+    # own directive is one that ticked the box off.
+    foreach my $line ( split( "\n", $template ) ) {
+        next                if $line =~ m/^\s*$/;
+        next                if $line =~ m/^\s*<!--/;
+        $shown{wrapper} = 1 if $line =~ m/^\s*<div\s+class="post\b/;
+        last;
+    }
+
+    return \%shown;
+}
+
 # The custom fields of a type, as rows the wizard would have submitted.
 # The custom fields of a type, as rows the wizard would have submitted.
 sub _wizard_fields_of ( $schema, $meta, $includes = {} ) {
 sub _wizard_fields_of ( $schema, $meta, $includes = {} ) {
     my $properties = $schema->{properties} // {};
     my $properties = $schema->{properties} // {};

+ 37 - 0
t/Trog-DataModule-schema.t

@@ -547,6 +547,43 @@ subtest 'a type reads back as the form that made it' => sub {
     ok( !exists $by_name{visibility}, 'any of them' );
     ok( !exists $by_name{visibility}, 'any of them' );
 };
 };
 
 
+subtest 'the checkboxes outside the editor come back from the template too' => sub {
+    set_components();
+
+    # A hand-written type: a wrapper div, both display blocks, and the
+    # multi-page body form.  Its sidecar records none of that, because nothing
+    # but a generated one ever did.
+    set_sidecars(
+        'deck.json' => sidecar(),
+        'deck.tx'   => <<'TEMPLATE',
+<div class="post <: $style :>">
+    :if ( !$post.addpost ) {
+        : include "post_title.tx";
+        : include "post_tags.tx";
+    : }
+        : include "form_multi.tx";
+</div>
+TEMPLATE
+
+        # And one which opens with its own markup instead of the wrapper, and
+        # shows neither block -- as file.tx and profile.tx really do.
+        'bare.json' => sidecar(),
+        'bare.tx'   => qq|<!-- a comment first, which is not the wrapper -->\n:if ( !\$post.addpost ) {\n: }\n        : include "form_common.tx";\n|,
+    );
+
+    my $types = Trog::Routes::HTML::_wizard_types( [ 'deck.tx', 'bare.tx' ] );
+
+    is( $types->{'deck.tx'}{wrapper},        1,               "the wrapper div is seen where a type opens with one" );
+    is( $types->{'deck.tx'}{inc_post_title}, 1,               "as is the post title block" );
+    is( $types->{'deck.tx'}{inc_post_tags},  1,               "and the post tag block" );
+    is( $types->{'deck.tx'}{body_form},      'form_multi.tx', "and which body form carries the post" );
+
+    is( $types->{'bare.tx'}{wrapper},        0,                'a type which opens with its own markup has the box clear' );
+    is( $types->{'bare.tx'}{inc_post_title}, 0,                'and the blocks it does not include are clear' );
+    is( $types->{'bare.tx'}{inc_post_tags},  0,                'both of them' );
+    is( $types->{'bare.tx'}{body_form},      'form_common.tx', 'and a single page body form is read back as one' );
+};
+
 subtest 'the types are handed to the page as JSON it cannot break out of' => sub {
 subtest 'the types are handed to the page as JSON it cannot break out of' => sub {
     no warnings qw{once};
     no warnings qw{once};
 
 

+ 161 - 0
t/Trog-Routes-HTML-post-wizard-browser.t

@@ -0,0 +1,161 @@
+#!/usr/bin/env perl
+
+# Drives the post type wizard in a real browser, against a running tCMS.
+#
+# The "Start from" dropdown is two halves which nothing below the browser can
+# check actually meet: post_wizard() writes every type that exists into the page
+# as JSON, and fillFromType() in post_wizard.js applies the one you picked to the
+# form.  A unit test can prove the JSON is right and still tell you nothing about
+# what an admin sees, and what an admin sees is where every bug in this feature
+# has lived so far.
+#
+# What is asserted is the state of the form after picking a type: which canned
+# boxes are ticked, which body form is selected, and which rows turn up under
+# Custom Fields.  The expectations below are the stock post types, read off the
+# templates in www/templates/html/components/forms -- so a theme which overrides
+# one of them will make this fail, which is why it is opt-in.
+#
+# Needs a running server and an admin to log in as, and skips without them:
+#
+#   TCMS_TEST_URL=http://localhost:5000 \
+#   TCMS_TEST_USER=someadmin TCMS_TEST_PASS=hunter2 \
+#     prove -Ilib t/Trog-Routes-HTML-post-wizard-browser.t
+#
+# playwright_server needs its node deps (uuid, playwright, express) somewhere
+# node resolves them from -- its own directory, /usr/local/lib/node_modules, or
+# NODE_PATH.  A global install under nvm is not one of those places.
+
+use strict;
+use warnings;
+
+use FindBin;
+use lib "$FindBin::Bin/../lib";
+
+use Test::More;
+
+my $URL  = $ENV{TCMS_TEST_URL} || 'http://localhost:5000';
+my $USER = $ENV{TCMS_TEST_USER};
+my $PASS = $ENV{TCMS_TEST_PASS};
+my $TYPE = $ENV{TCMS_TEST_BROWSER} || 'firefox';
+
+plan skip_all => 'set TCMS_TEST_USER and TCMS_TEST_PASS to run the browser tests' unless $USER && $PASS;
+
+# Playwright checks its node deps in a BEGIN, so this is a require rather than a
+# use: a kit that cannot start the server should skip rather than fail to
+# compile.
+eval { require Playwright; 1 } or plan skip_all => "Playwright will not load: $@";
+
+# Every stock post type, as the wizard ought to read it back.  'boxes' is every
+# checkbox the wizard offers; 'fields' is what should be left under Custom
+# Fields once the canned blocks have been accounted for.
+my %EXPECT = (
+    'blog.tx' => {
+        body   => 'form_common.tx',
+        fields => [],
+        boxes  => { wrapper => 1, inc_post_title => 1, inc_post_tags => 1, inc_preview => 1, inc_tags => 1, inc_aliases => 1, inc_attachments => 0 },
+    },
+
+    # The multi-page one: reopening it used to hand back form_common.tx, so
+    # saving it again silently turned a slide deck into a single page.
+    'presentation.tx' => {
+        body   => 'form_multi.tx',
+        fields => ['video_href'],
+        boxes  => { wrapper => 1, inc_post_title => 1, inc_post_tags => 1, inc_preview => 1, inc_tags => 1, inc_aliases => 1, inc_attachments => 1 },
+    },
+
+    # No wrapper -- it opens with its own markup so it can tile -- and its avatar
+    # upload is its own rather than preview.tx, so preview really is a field of
+    # this type and should still be offered as one.
+    'profile.tx' => {
+        body   => 'form_common.tx',
+        fields => [qw{contact_email display_name preview preview_file user_acls username wallpaper wallpaper_file}],
+        boxes  => { wrapper => 0, inc_post_title => 1, inc_post_tags => 0, inc_preview => 0, inc_tags => 0, inc_aliases => 1, inc_attachments => 0 },
+    },
+
+    # A datasource type: built rather than written, so it has no editor at all,
+    # and the preview it declares is a console capture of its own.
+    'guests.tx' => {
+        body   => 'form_common.tx',
+        fields => [qw{preview hypervisors}],
+        boxes  => { wrapper => 1, inc_post_title => 0, inc_post_tags => 0, inc_preview => 0, inc_tags => 0, inc_aliases => 0, inc_attachments => 0 },
+    },
+);
+
+my @BOXES = qw{wrapper inc_post_title inc_post_tags inc_preview inc_tags inc_aliases inc_attachments};
+
+# Locators throughout rather than select(): a select() hands back a handle to
+# the element as it was at that moment, and the login page settles after
+# 'networkidle' often enough that filling one raced with the DOM it came from.
+# A locator resolves when it is acted on instead.
+my $handle  = Playwright->new();
+my $browser = $handle->launch( headless => 1, type => $TYPE );
+my $page    = $browser->newPage();
+
+END {
+    eval { $page->close() }    if $page;
+    eval { $browser->close() } if $browser;
+    eval { $handle->quit() }   if $handle;
+}
+
+sub field_names {
+    return $page->evaluate('return Array.from(document.querySelectorAll("#wizard-params input[name=param_name]")).map(function (i) { return i.value; });');
+}
+
+subtest 'an admin can reach the wizard' => sub {
+
+    # Posted rather than typed into the login page.  The session lands in the
+    # page's own cookie jar either way, and this keeps the test off a page it is
+    # not about: on a successful login jsalert.tx schedules a window.location to
+    # $to half a second after DOMContentLoaded, so the login form navigates out
+    # from under anything still filling it in.
+    my $res = $page->request->fetch(
+        "$URL/auth",
+        {
+            method => 'POST',
+            form   => { app => 'login', to => '', username => $USER, password => $PASS },
+        }
+    );
+    is( $res->status(), 200, "logged in at $URL" ) or BAIL_OUT("no tCMS answering at $URL, or those credentials are wrong");
+
+    $res = $page->goto( "$URL/admin/wyzzerdd", { waitUntil => 'networkidle' } );
+    is( $res->status(), 200, 'the wizard renders' );
+
+    is( $page->locator('#wizard-existing')->count(), 1, "the 'Start from' dropdown is there" )
+      or BAIL_OUT("not logged in as an admin -- check TCMS_TEST_USER");
+};
+
+foreach my $form ( sort keys(%EXPECT) ) {
+    subtest "picking $form fills the form in the way that type was built" => sub {
+        my $expect = $EXPECT{$form};
+
+        # Back to blank between types, so nothing here can pass on what the
+        # previous type left in the form rather than on what this one says.
+        $page->locator('#wizard-existing')->selectOption( { value => '' } );
+        is_deeply( field_names(), [''], 'a blank pick leaves one empty custom field row' );
+
+        $page->locator('#wizard-existing')->selectOption( { value => $form } );
+
+        foreach my $box (@BOXES) {
+            my $want = $expect->{boxes}{$box};
+            my $got  = $page->locator(qq{#postTypeWizard [name="$box"]})->isChecked() ? 1 : 0;
+            is( $got, $want, "$box is " . ( $want ? 'ticked' : 'clear' ) );
+        }
+
+        is( $page->locator('#postTypeWizard [name="body_form"]')->inputValue(), $expect->{body}, 'the body form is the one the type carries' );
+
+        # Only the named rows are asserted: fillFromType() always leaves an
+        # empty one behind to type the next field into, and a type with no
+        # custom fields at all gets that row and nothing else.
+        is_deeply(
+            [ sort grep { length } @{ field_names() } ],
+            [ sort @{ $expect->{fields} } ],
+            'and only the fields somebody typed in are offered as custom ones'
+        );
+
+        # Saving under the same name is the point of picking one, and the save
+        # is refused outright without this.
+        ok( $page->locator('#postTypeWizard [name="overwrite"]')->isChecked(), 'overwrite is ticked for you' );
+    };
+}
+
+done_testing();