Problem:

Currently the 'Revision information' vertical tab summary text is linked to the state of a nodes 'Create new revision' checkbox. If a user without 'administer content' permissions edits a node type that has revisioning on by default, that checkbox doesn't appear. As a result the tab summary will incorrectly tell the user that 'No revision' will be created.

Fix:

Attach the summary text to the checkbox state only when the checkbox is available. When it is not, attach it to the presence of the log textarea. For non-'administer content' users the log will only appear for nodes that have 'create new revision' on by default.

Patch attached.

CommentFileSizeAuthor
#16 revision_information_incorrect_summary-7.patch1.31 KBalexjarvis
#13 revision_information_incorrect_summary-6.patch1.23 KBalexjarvis
#8 revision_information_incorrect_summary-5.patch1.08 KBalexjarvis
#5 revision_information_incorrect_summary-4.patch1.08 KBalexjarvis
#3 revision_information_incorrect_summary-3.patch1.11 KBalexjarvis
#1 revision_information_incorrect_summary-2.patch1.07 KBalexjarvis
revision_information_incorrect_summary.patch1.06 KBalexjarvis

Comments

alexjarvis’s picture

alexjarvis commented
StatusFileSize
new revision_information_incorrect_summary-2.patch1.07 KB

Small change: added leading space to comment per code standard.

aspilicious’s picture

aspilicious commented

Can you wrap the comment in 80 characters?

==> make it a multiple line comment, break as close to 80 characters

alexjarvis’s picture

alexjarvis commented
StatusFileSize
new revision_information_incorrect_summary-3.patch1.11 KB

Wrapped comment.

retester2010’s picture

retester2010 commented
Status: Needs review » Needs work 
+++ modules/node/node.js	23 Mar 2010 16:02:04 -0000
@@ -5,7 +5,16 @@
+      var no_checkbox_log_exists = false;
+      ¶
+      // If #edit-revision isn't checked, see if it even exists. ¶
+      // If #edit-revision doesn't exist, fall back on existance of #edit-log.
+      if (!create_revision_checked && $('#edit-revision', context).size() == 0) { ¶
+        no_checkbox_log_exists = $('#edit-log', context).size() > 0;
+      }
+      ¶
+      return create_revision_checked || no_checkbox_log_exists ? ¶

trailing white spaces

40 critical left. Go review some!

alexjarvis’s picture

alexjarvis commented
Status: Needs work » Needs review
StatusFileSize
new revision_information_incorrect_summary-4.patch1.08 KB

Trailing white space removed. Also restructured logic for increased clarity.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented

2 nitpicks:

  • a space between if and its condition
  • a space after the // beginning a comment

Patch works great, nice catch!

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Needs work
alexjarvis’s picture

alexjarvis commented
Status: Needs work » Needs review
StatusFileSize
new revision_information_incorrect_summary-5.patch1.08 KB

Fixed.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented

This looks correct to me but it would be nice to get another thumbs up from casey or so.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
+++ modules/node/node.js	2 Aug 2010 04:27:24 -0000
@@ -5,9 +5,14 @@
+      // Return 'New revision' if the 'Create new revision' checkbox is checked,
+      // or if the checkbox doesn't exist, but the revision log does.

Good start at explaining, but I had to think twice (and others would have to look up actual Node module code) to understand *why*.

+++ modules/node/node.js	2 Aug 2010 04:27:24 -0000
@@ -5,9 +5,14 @@
+      if ($('#edit-revision', context).is(':checked') ||
+         ($('#edit-revision', context).length == 0 && $('#edit-log', context).length != 0)) {

1) Just checking for $foo.length and !$foo.length is sufficient.

2) Can we write the condition on one line, please?

Powered by Dreditor.

alexjarvis’s picture

alexjarvis commented
Status: Needs work » Needs review
StatusFileSize
new revision_information_incorrect_summary-6.patch1.23 KB

Fixed.

JacobSingh’s picture

JacobSingh commented
Status: Needs review » Reviewed & tested by the community

Looks fine to me. The comment is still a little confusing, but then the use case is too.

ksenzee’s picture

ksenzee
she/her
commented
Status: Reviewed & tested by the community » Needs work

We should be assigning the result of $('#edit-revision', context) to a variable, not calling it twice.

alexjarvis’s picture

alexjarvis commented
Status: Needs work » Needs review
StatusFileSize
new revision_information_incorrect_summary-7.patch1.31 KB

Assigned to variable and updated selectors to comply with #955610: Impossible to have multiple node forms on one page due to fixed IDs.

ksenzee’s picture

ksenzee
she/her
commented
Status: Needs review » Reviewed & tested by the community

Looks good, works as designed. A+++ would review again.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Since ksenzee can make jQuery bend to her every whim, I trust her opinion on this. :)

Committed to HEAD. Thanks! Great job, alexjarvis!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Thanks for taking this over, ksenzee! :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.