Changeset 928

Timestamp:

03/10/07 23:49:39 (21 months ago)

Author:

kake

Message:

Write tests for and fix bugs #48 and #173; in the process, move preview_node() and edit_node() from wiki.cgi into OpenGuides.pm, and add more functionality to OpenGuides::Test.

Location:

trunk

Files:

• Build.PL (modified) (1 diff)
• Changes (modified) (1 diff)
• MANIFEST (modified) (2 diffs)
• README.CSS (modified) (12 diffs)
• lib/OpenGuides.pm (modified) (5 diffs)
• lib/OpenGuides/Template.pm (modified) (1 diff)
• lib/OpenGuides/Test.pm (modified) (2 diffs)
• templates/edit_form.tt (modified) (10 diffs)
• wiki.cgi (modified) (2 diffs)

trunk/Build.PL

@@ -322 +322 @@
                       "differences.tt",
                       "display_metadata.tt",
-                      "edit_conflict.tt",
                       "edit_form.tt",
                       "error.tt",

trunk/Changes

@@ -3 +3 @@
 More detailed changelogs can be found at
 http://dev.openguides.org/log/trunk
+
+0.59    
+        Move preview_node() and edit_node() from wiki.cgi into OpenGuides.pm
+        Remove edit_conflict.tt - use edit_form.tt instead to reduce
+          duplication.
+        Make sure to always pass the config object into the templates.
+        Add some extra test utilities to OpenGuides::Test
+        Write tests for and fix:
+          #48 (Edit conflict page erroneously converts lat/lon to os_x, os_y).
+          #173 (edit conflict form doesn't let you edit everything).
 
 0.58    21 December 2006

trunk/MANIFEST

@@ -39 +39 @@
 templates/differences.tt
 templates/display_metadata.tt
-templates/edit_conflict.tt
 templates/edit_form.tt
 templates/error.tt
@@ -95 +94 @@
 t/40_search_as_feed.t
 t/41_deletion.t
+t/42_edit_conflict.t
 t/51_display_node.t
 t/52_display_diffs.t

trunk/README.CSS

@@ -26 +26 @@
 
 div.warning_text
-    Used in: edit_conflict.tt
+    Used in: edit_form.tt
     Purpose: Warns the user that there is an edit conflict
 
@@ -61 +61 @@
 span.node_name
     Used in: backlink_results.tt delete_confirm.tt delete_done.tt
-             differences.tt node_history.tt, edit_conflict.tt
+             differences.tt node_history.tt
     Purpose: Inlines a node name in a snippet of explanatory text,
              without linking to it.
 
 td.label
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: Defines a label for a field
 
@@ -99 +99 @@
 div#maincontent
     Used in: backlink_results.tt, delete_confirm.tt, delete_done.tt,
-             delete_password_wrong.tt, edit_conflict.tt, edit_form.tt,
+             delete_password_wrong.tt, edit_form.tt,
              home_node.tt, newpage.tt, node.tt, node_history.tt, preferences.tt,
              recent_changes.tt, search_results.tt, site_index.tt,
@@ -195 +195 @@
 
 input#address
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the address of the node
 
@@ -203 +203 @@
 
 input#fax
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the fax number of the node
 
@@ -216 +216 @@
 
 input#hours
-    Used in: edit_conflit.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the opening hours of a node
     
@@ -225 +225 @@
 
 input#map_link
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the map link of the node
 
@@ -238 +238 @@
 
 input#os_x
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the OS X coordinate of the node
 
 input#os_y
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the OS Y coordinate of the node
 
@@ -250 +250 @@
 
 input#phone
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the telephone number of the node
 
 input#postcode
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the postcode of the node
         
@@ -284 +284 @@
 
 input#website
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An input field for the web site of the node
 
@@ -301 +301 @@
 
 textarea#categories
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: An box to enter the categories of the node
 
 textarea#content_textarea
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: A box to enter the main conent of the node
     Note: ID doesn't match class, as IDs have to be globally unique, and we
@@ -311 +311 @@
 
 textarea#locales
-    Used in: edit_conflict.tt, edit_form.tt
+    Used in: edit_form.tt
     Purpose: A box to enter the locales of the node
 

trunk/lib/OpenGuides.pm

@@ -318 +318 @@
         return $output if $return_output;
         print $output;
+    }
+}
+
+=item B<display_edit_form>
+
+  $guide->display_edit_form(
+                             id => "Vivat Bacchus",
+                           );
+
+Display an edit form for the specified node.  As with other methods, the
+C<return_output> parameter can be used to return the output instead of
+printing it to STDOUT.
+
+=cut
+
+sub display_edit_form {
+    my ($self, %args) = @_;
+    my $return_output = $args{return_output} || 0;
+    my $config = $self->config;
+    my $wiki = $self->wiki;
+    my $node = $args{id};
+    my %node_data = $wiki->retrieve_node($node);
+    my ($content, $checksum) = @node_data{ qw( content checksum ) };
+    my %cookie_data = OpenGuides::CGI->get_prefs_from_cookie(config=>$config);
+
+    my $username = $self->get_cookie( "username" );
+    my $edit_type = $self->get_cookie( "default_edit_type" ) eq "normal"
+                        ? "Normal edit"
+                        : "Minor tidying";
+
+    my %metadata_vars = OpenGuides::Template->extract_metadata_vars(
+                             wiki     => $wiki,
+                             config   => $config,
+                 metadata => $node_data{metadata} );
+
+    $metadata_vars{website} ||= 'http://';
+    my $moderate = $wiki->node_required_moderation($node);
+
+    my %tt_vars = ( content         => CGI->escapeHTML($content),
+                    checksum        => CGI->escapeHTML($checksum),
+                    %metadata_vars,
+                    config          => $config,
+                    username        => $username,
+                    edit_type       => $edit_type,
+                    moderate        => $moderate,
+                    deter_robots    => 1,
+    );
+
+    my $output = $self->process_template(
+                                          id            => $node,
+                                          template      => "edit_form.tt",
+                                          tt_vars       => \%tt_vars,
+                                        );
+    return $output if $return_output;
+    print $output;
+}
+
+=item B<preview_edit>
+
+  $guide->display_edit_form(
+                             id      => "Vivat Bacchus",
+                             cgi_obj => $q,
+                           );
+
+Preview the edited version of the specified node.  As with other methods, the
+C<return_output> parameter can be used to return the output instead of
+printing it to STDOUT.
+
+=cut
+
+sub preview_edit {
+    my ($self, %args) = @_;
+    my $node = $args{id};
+    my $q = $args{cgi_obj};
+    my $return_output = $args{return_output};
+    my $wiki = $self->wiki;
+    my $config = $self->config;
+
+    my $content  = $q->param('content');
+    $content     =~ s/\r\n/\n/gs;
+    my $checksum = $q->param('checksum');
+
+    my %new_metadata = OpenGuides::Template->extract_metadata_vars(
+                                               wiki                 => $wiki,
+                                               config               => $config,
+                                               cgi_obj              => $q,
+                                               set_coord_field_vars => 1,
+    );
+    foreach my $var ( qw( username comment edit_type ) ) {
+        $new_metadata{$var} = $q->escapeHTML($q->param($var));
+    }
+
+    if ($wiki->verify_checksum($node, $checksum)) {
+        my $moderate = $wiki->node_required_moderation($node);
+        my %tt_vars = (
+            %new_metadata,
+            config                 => $config,
+            content                => $q->escapeHTML($content),
+            preview_html           => $wiki->format($content),
+            preview_above_edit_box => $self->get_cookie(
+                                                   "preview_above_edit_box" ),
+            checksum               => $q->escapeHTML($checksum),
+            moderate               => $moderate
+        );
+        my $output = process_template(
+                                       id => $node,
+                                       template => "edit_form.tt",
+                                       tt_vars => \%tt_vars,
+                                     );
+        return $output if $args{return_output};
+        print $output;
+    } else {
+        return $self->_handle_edit_conflict(
+                                             id            => $node,
+                                             content       => $content,
+                                             new_metadata  => \%new_metadata,
+                                             return_output => $return_output,
+                                           );
     }
 }
@@ -1040 +1158 @@
     my $checksum = $q->param("checksum");
 
-    my %metadata = OpenGuides::Template->extract_metadata_vars(
+    my %new_metadata = OpenGuides::Template->extract_metadata_vars(
         wiki    => $wiki,
         config  => $config,
@@ -1046 +1164 @@
     );
 
-    delete $metadata{website} if $metadata{website} eq 'http://';
-
-    $metadata{opening_hours_text} = $q->param("hours_text") || "";
+    delete $new_metadata{website} if $new_metadata{website} eq 'http://';
+
+    $new_metadata{opening_hours_text} = $q->param("hours_text") || "";
 
     # Pick out the unmunged versions of lat/long if they're set.
     # (If they're not, it means they weren't munged in the first place.)
-    $metadata{latitude} = delete $metadata{latitude_unmunged}
-        if $metadata{latitude_unmunged};
-    $metadata{longitude} = delete $metadata{longitude_unmunged}
-        if $metadata{longitude_unmunged};
+    $new_metadata{latitude} = delete $new_metadata{latitude_unmunged}
+        if $new_metadata{latitude_unmunged};
+    $new_metadata{longitude} = delete $new_metadata{longitude_unmunged}
+        if $new_metadata{longitude_unmunged};
 
     # Check to make sure all the indexable nodes are created
@@ -1063 +1181 @@
         $self->_autoCreateCategoryLocale(
                                           id       => $node,
-                                          metadata => \%metadata
+                                          metadata => \%new_metadata
         );
     }
     
     foreach my $var ( qw( summary username comment edit_type ) ) {
-        $metadata{$var} = $q->param($var) || "";
-    }
-    $metadata{host} = $ENV{REMOTE_ADDR};
+        $new_metadata{$var} = $q->param($var) || "";
+    }
+    $new_metadata{host} = $ENV{REMOTE_ADDR};
 
     # Wiki::Toolkit::Plugin::RSS::ModWiki wants "major_change" to be set.
-    $metadata{major_change} = ( $metadata{edit_type} eq "Normal edit" )
-                            ? 1
-                            : 0;
-
-    my $written = $wiki->write_node($node, $content, $checksum, \%metadata );
+    $new_metadata{major_change} = ( $new_metadata{edit_type} eq "Normal edit" )
+                                    ? 1
+                                    : 0;
+
+    my $written = $wiki->write_node( $node, $content, $checksum,
+                                     \%new_metadata );
 
     if ($written) {
@@ -1084 +1203 @@
         print $output;
     } else {
-        my %node_data = $wiki->retrieve_node($node);
-        my %tt_vars = ( checksum       => $node_data{checksum},
-                        new_content    => $content,
-                        stored_content => $node_data{content} );
-        foreach my $mdvar ( keys %metadata ) {
-            if ($mdvar eq "locales") {
-                $tt_vars{"stored_$mdvar"} = $node_data{metadata}{locale};
-                $tt_vars{"new_$mdvar"}    = $metadata{locale};
-            } elsif ($mdvar eq "categories") {
-                $tt_vars{"stored_$mdvar"} = $node_data{metadata}{category};
-                $tt_vars{"new_$mdvar"}    = $metadata{category};
-            } elsif ($mdvar eq "username" or $mdvar eq "comment"
-                      or $mdvar eq "edit_type" ) {
-                $tt_vars{$mdvar} = $metadata{$mdvar};
-            } else {
-                $tt_vars{"stored_$mdvar"} = $node_data{metadata}{$mdvar}[0];
-                $tt_vars{"new_$mdvar"}    = $metadata{$mdvar};
-            }
-        }
-        return %tt_vars if $args{return_tt_vars};
-        my $output = $self->process_template(
-                                              id       => $node,
-                                              template => "edit_conflict.tt",
-                                              tt_vars  => \%tt_vars,
-                                            );
-        return $output if $args{return_output};
-        print $output;
-    }
+        return $self->_handle_edit_conflict(
+                                             id            => $node,
+                                             content       => $content,
+                                             new_metadata  => \%new_metadata,
+                                             return_output => $return_output,
+                                           );
+    }
+}
+
+sub _handle_edit_conflict {
+    my ($self, %args) = @_;
+    my $return_output = $args{return_output} || 0;
+    my $config = $self->config;
+    my $wiki = $self->wiki;
+    my $node = $args{id};
+    my $content = $args{content};
+    my %new_metadata = %{$args{new_metadata}};
+
+    my %node_data = $wiki->retrieve_node($node);
+    my %tt_vars = ( checksum       => $node_data{checksum},
+                    new_content    => $content,
+                    content        => $node_data{content} );
+    my %old_metadata = OpenGuides::Template->extract_metadata_vars(
+                                           wiki     => $wiki,
+                                           config   => $config,
+                                           metadata => $node_data{metadata} );
+    # Make sure we look at all variables.
+    my @tmp = (keys %new_metadata, keys %old_metadata );
+    my %tmp_hash = map { $_ => 1; } @tmp;
+    my @all_vars = keys %tmp_hash;
+
+    foreach my $mdvar ( keys %new_metadata ) {
+        if ($mdvar eq "locales") {
+            $tt_vars{$mdvar} = $old_metadata{locales};
+            $tt_vars{"new_$mdvar"} = $new_metadata{locale};
+        } elsif ($mdvar eq "categories") {
+            $tt_vars{$mdvar} = $old_metadata{categories};
+            $tt_vars{"new_$mdvar"} = $new_metadata{category};
+        } elsif ($mdvar eq "username" or $mdvar eq "comment"
+                  or $mdvar eq "edit_type" ) {
+            $tt_vars{$mdvar} = $new_metadata{$mdvar};
+        } else {
+            $tt_vars{$mdvar} = $old_metadata{$mdvar};
+            $tt_vars{"new_$mdvar"} = $new_metadata{$mdvar};
+        }
+    }
+
+    $tt_vars{coord_field_1} = $old_metadata{coord_field_1};
+    $tt_vars{coord_field_2} = $old_metadata{coord_field_2};
+    $tt_vars{coord_field_1_value} = $old_metadata{coord_field_1_value};
+    $tt_vars{coord_field_2_value} = $old_metadata{coord_field_2_value};
+    $tt_vars{"new_coord_field_1_value"}
+                                = $new_metadata{$old_metadata{coord_field_1}};
+    $tt_vars{"new_coord_field_2_value"}
+                                = $new_metadata{$old_metadata{coord_field_2}};
+
+    $tt_vars{conflict} = 1;
+    return %tt_vars if $args{return_tt_vars};
+    my $output = $self->process_template(
+                                          id       => $node,
+                                          template => "edit_form.tt",
+                                          tt_vars  => \%tt_vars,
+                                        );
+    return $output if $args{return_output};
+    print $output;
 }
 

trunk/lib/OpenGuides/Template.pm

@@ -147 +147 @@
 
     my $tt_vars = {
+        config                => $config,
         site_name             => $config->site_name,
         cgi_url               => $script_name,

trunk/lib/OpenGuides/Test.pm

@@ -93 +93 @@
 would come from a CGI form, eg lines in a textarea are separated by C<\r\n>.
 
+This method will automatically grab the checksum from the database, so
+even if the node already exists your data will still be written.  If you
+don't want this behaviour (for example, if you're testing edit conflicts)
+then pass in a true value to the C<omit_checksum> parameter:
+
+  OpenGuides::Test->write_data(
+                                guide         => $guide,
+                                node          => "Crabtree Tavern",
+                                omit_checksum => 1,
+                              );
+
+If you want to grab the output, pass a true value to C<return_output>:
+
+  my $output = OpenGuides::Test->write_data(
+                                             guide        => $guide,
+                                             node         => "Crabtree Tavern",
+                                             return_output => 1,
+                                           );
+
+Similarly, if you pass a true value to C<return_tt_vars>, the return value
+will be the variables which would have been passed to the template for output:
+
+  my %vars = OpenGuides::Test->write_data(
+                                             guide        => $guide,
+                                             node         => "Crabtree Tavern",
+                                             return_tt_vars => 1,
+                                           );
+
 =cut
 
@@ -123 +151 @@
     
     # Get the checksum of the current contents if necessary.
-    my $wiki = $args{guide}->wiki;
-    if ( $wiki->node_exists( $args{node} ) ) {
-        my %data = $wiki->retrieve_node( $args{node} );
-        $q->param( -name => "checksum", -value => $data{checksum} );
+    unless ( $args{omit_checksum} ) {
+        my $wiki = $args{guide}->wiki;
+        if ( $wiki->node_exists( $args{node} ) ) {
+            my %data = $wiki->retrieve_node( $args{node} );
+            $q->param( -name => "checksum", -value => $data{checksum} );
+        }
     }
-
-    $args{guide}->commit_node(
-                               return_output => 1,
-                               id => $args{node},
-                               cgi_obj => $q,
-                             );
+ 
+    if ( $args{return_output} ) {
+        return $args{guide}->commit_node(
+                                          return_output => 1,
+                                          id => $args{node},
+                                          cgi_obj => $q,
+                                        );
+    } elsif ( $args{return_tt_vars} ) {
+        return $args{guide}->commit_node(
+                                          return_tt_vars => 1,
+                                          id => $args{node},
+                                          cgi_obj => $q,
+                                        );
+    } else {
+        $args{guide}->commit_node(
+                                   id => $args{node},
+                                   cgi_obj => $q,
+                                 );
+    }
 }
 

trunk/templates/edit_form.tt

@@ +1 @@
+[% USE CGI %]
 [% INCLUDE header.tt page_title = "Edit $node_name - $site_name" %]
 <div id="content">
@@ -4 +5 @@
 <div id="maincontent">
 <h1 align="center">Edit &quot;[% node_name %]&quot;</h1>
+
+[% IF conflict %]
+  <div class="warning_text">
+    Someone has committed changes to this node since you began editing.
+    You will need to manually merge your changes into the new version.
+  </div>
+[% END %]
 
 [% IF preview_html AND preview_above_edit_box %]
@@ -15 +23 @@
   <table summary="Form for editing page attributes">
     <tr>
-      <td colspan="2">
+      <td colspan="[% IF conflict %]3[% ELSE %]2[% END %]">
         [%# Put the td outside the TRY-CATCH because TT will print everything
             up to where it gets the error - avoid mismatched tags. %]
@@ -39 +47 @@
     </tr>
     <tr>
-      <td colspan="2" align="center"><a href="[% cgi_url %]?[% node_param %]">(cancel edit)</a></td>
+      <td colspan="[% IF conflict %]3[% ELSE %]2[% END %]" align="center"><a href="[% cgi_url %]?[% node_param %]">(cancel edit)</a></td>
     </tr>
     <tr>
-      <td colspan="2">
+      <td colspan="[% IF conflict %]3[% ELSE %]2[% END %]">
         <fieldset>
           <legend>Main information (required)</legend>
@@ -49 +57 @@
               <td class="label"><label for="content">Content:</label></td>
               <td><textarea name="content" id="content_textarea" rows="21" cols="70" wrap="virtual">[% content %]</textarea></td>
+              [% IF conflict %]
+                <td><small>[% CGI.escapeHTML(new_content) %]</small></td>
+              [% END %]
             </tr>
             <tr>
@@ -57 +68 @@
               <td><textarea name="locales" id="locales" rows="5" cols="70">[% FOREACH locale = locales %][% locale.name %]
 [% END %]</textarea></td>
+              [% IF conflict %]
+                <td>[% FOREACH locale = new_locales %][% CGI.escapeHTML(locale) %]<br/>[% END %]</td>
+              [% END %]
             </tr>
             <tr>
@@ -62 +76 @@
               <td><textarea name="categories" id="categories" rows="5" cols="70">[% FOREACH category = categories %][% category.name %]
 [% END %]</textarea></td>
+              [% IF conflict %]
+                <td>[% FOREACH category = new_categories %][% CGI.escapeHTML(category) %]<br/>[% END %]</td>
+              [% END %]
             </tr>
           </table>
@@ -76 +93 @@
                 [% INCLUDE node_photo_notes.tt %]
               </td>
+              [% IF conflict %]
+                <td>[% CGI.escapeHTML(new_node_image) %]</td>
+              [% END %]
             </tr>
 [% END %]