the other day, sepeck hipped me to the amazingly useful book outliner functionality. for example (if you're a site admin @ d.o): http://drupal.org/admin/node/book/257

so, i fixed up a few things on the theme update pages for the switch to 2-digit version numbers, and hit the "Save book pages" button. out of curiosity, i wanted to see what the log message was going to say, and was sadly disappointed. for example:

http://drupal.org/node/25297/revisions

30/09/2006 - 18:17 by dww
themes shouldn't use theme_add_style internally (see bug 52508)

07/03/2006 - 18:39 by TDobes
themes shouldn't use theme_add_style internally (see bug 52508)

this appears to be another instance of a bug i thought i had fixed in http://drupal.org/node/39124

however, i'm submitting it as a new issue, since it's really a different thing: that was the book edit form, this is the outliner.

when i have a chance, i'll post a patch for this. if someone else is inspired and gets it done first, all the better. ;)

CommentFileSizeAuthor
#20 book.d5.log-message.patch1.28 KBFreso
#16 book-log-message-86857-16.patch1.37 KBwebchick
#15 book-log-message-86857-15.patch1.37 KBwebchick
#14 book-log-message-86857-14.patch822 byteswebchick
#11 book-log-message-86857-10.patch1.65 KBwebchick
#4 book-log-6x-86857-4.patch746 bytespwolanin

Comments

magico’s picture

magico commented

@dww: any news about this. The links point to drupal, an you provide some steps to recreate this?

dww’s picture

dww
we/he/they
commented

this is still broken.
i've had no time to fix it.

here's how to reproduce:

  1. enable book.module and turn on 'make new revision' as the default workflow for book nodes
  2. make a book (you'd probably see the bug with a single page, but a few children pages might be a good idea)
  3. make at least 1 revision of a book page, and put something in the 'log' field for this revision.
  4. go to the admin books section (in 4.7.x it's [site]/admin/node/book -- i haven't looked for it in the new admin layout)
  5. use the UI to change stuff about your book page (or a set of book pages) -- weights, titles, etc.
  6. inspect the revisions tab for your test book page. you'll see that the new revision created from the admin book outliner UI has the old log message from your previous edit.

at the very least, the log message should be blank in this case. better yet, it should automatically set to something like "Changed via administrative book outliner interface". in D6, we should probably add a log field textarea to this UI, and use that.

cheers,
-derek

pwolanin’s picture

pwolanin commented
Version: 4.7.3 » 6.x-dev

This bug seems to occur still in 6.x, even though the dedicated node type is gone.

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new book-log-6x-86857-4.patch746 bytes

here's a patch for 6.x

pwolanin’s picture

pwolanin commented

bump. This is a trivial patch, so the only question is whether to use a message as proposed, or just put an empty string.

Freso’s picture

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

I could reproduce the bug without a checkout from latest CVS. Then fetched the patch, which applied cleanly. Trying to reproduce the bug fails, as the patch works as advertised. Yay! :)

Some comments though:

  1. I'd like a period appended to the log text. I don't know what's the custom for these kinds of messages, but AFAIK it's (more) grammatically correct to have it.
  2. This would probably require a somewhat bigger patch, but I'd rather like to see one being able to enter a log message by themselves, instead of having the system generate one. (E.g. Moving "create patches" above "apply patches".)

The first one is rather minor and can be quickly fixed by the committer, but the second one, as noted, is probably a bigger patch. Since the present patch is better than the present situation, I'm marking RTBC, though still requesting a comment from someone on whether to open a new issue to allow one to enter a custom log message for changes made through admin/content/book.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

I'm not sure I like that fix. The wording of the message is not user-friendly, and I wonder why we should bother recording such a message.

pwolanin’s picture

pwolanin commented

@Dries - I'm fine with an empty log message too - the idea was just to indicate where/how the revision was generated.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Yes, it might still be a good idea to store if a change was made on the outline form I think. Having an empty log message looks cleaner, but does not retain what is possibly important information here. Although the log message could be a bit more user friendly, eg t('Changed on the book outline form at admin/content/book'). Including the path is also a bit tricky as it can be aliased or moved around, so a drupal_get_path_alias() call would make it provide more exact information there.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

My vote is for empty log message, personally. The only way the automated log message would be useful is if it said:

* Changed page weight from 7 to 18.
* Changed title from 'hello' to 'goodbye'

The fact that it was changed, and from where it was changed, isn't useful.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review
StatusFileSize
new book-log-message-86857-10.patch1.65 KB

Hm. Actually. Could we just do that?

Freso’s picture

Freso commented
Status: Needs review » Needs work

FWIW, my vote is for some message. With an empty message you'll have to look at the revision to say "Ahh, that was what I did then.", while you might far sooner come to the same revelation if it said that it was changed on this-and-this page (since that page would only have limited options for changing it in the first place).
Also, see my point no. 2 in comment #6.

Freso’s picture

Freso commented
Status: Needs work » Reviewed & tested by the community

webchick, I love you!

Title of page changed from Test: "wrong log message inserted when using book outliner", pt. 2 to Test: "wrong log message inserted when using book outliner", pt. 2 (w/ new patch). Weight of page changed from -3 to -2.

Patch applies cleanly and works wonderfully as advertised. The new log messages are clear and to-the-point. If anything, there's too much/specific info in the log message, but hey, I'm not complaining! :) Marking RTBC.
(I'd still like to be able to specify custom log messages, as previously stated, to explain motivation etc., but this is excellent for now. :))

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new book-log-message-86857-14.patch822 bytes

I realized we already have the original and new values, so no need to store them in the form definition.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
StatusFileSize
new book-log-message-86857-15.patch1.37 KB

Sorry. here's one with -up.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
StatusFileSize
new book-log-message-86857-16.patch1.37 KB

Ok. No more patch rolling before coffee. :P

I *think* this is good to go now.

Freso’s picture

Freso commented
Status: Needs work » Reviewed & tested by the community

Patch from #16 shows same behaviour as reported in comment #13, in addition to being leaner code. I believe it's RTBC, unless webchick has further objections to her patch. ;)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

This looks way better. Thanks for the patch, committed.

pwolanin’s picture

pwolanin commented
Version: 6.x-dev » 5.x-dev
Status: Fixed » Patch (to be ported)

great, thanks!

some version of this should be backported as well.

Freso’s picture

Freso commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new book.d5.log-message.patch1.28 KB

Here's an almost identical patch for D5, which hasn't been tested. So, please, test.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Can we do this? It changes strings in a stable release.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@webchick: There is no hard rule. We did this before, some strings were added/changed when it was necessary. It needs consideration in each case.

Freso’s picture

Freso commented

So, who should make the decision? Dries? Drumm?

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Bump: #20 still applies (without fuzz!) to DRUPAL-5. I just tested it and it all works as expected. I'd support this being committed -- yes it "breaks" the translations, but only in a very obscure place in the UI. ;) Therefore, I'm marking this RTBC, to put this on the radar for the core committers -- probably drumm will ultimately be the one to decide about this particular backport.

drumm’s picture

drumm
he/him
Location NY, US
commented
Category: bug » feature
Status: Reviewed & tested by the community » Closed (won't fix)

I would say this is a feature that could be implemented in a module for 5.x.