Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#11138 closed defect (fixed)

Data attribute inconsitency in trac.wiki.model.WikiPage while renaming a page

Reported by: Steffen Hoffmann Owned by: Steffen Hoffmann
Priority: normal Milestone: 0.12.6
Component: wiki system Version:
Severity: minor Keywords: listener resource
Cc: ryan.j.ollos@…
Release Notes:

Update WikiPage.resource.id when renaming a wiki page.

API Changes:

Description

When implementing an IWikiChangeListener for th:wiki:VotePlugin the initial draft didn't work as expected.

By comparing to known-good code for th:wiki:TagsPlugin I noticed, that there the new name is taken from page.name, while now I relied on the Resource object page.resource alone.

I feel that the current method WikiPage.rename is flawed since the initial commit in r9362 where it is updating only page.name but leaving page.resource unchanged. The WikiPage object passed to change listeners has both, the old and new name, and this is certainly unintended. However, it is easy to fix:

  • trac/wiki/model.py

    diff -u trac/wiki/model.py trac/wiki/model.py
    a b  
    189189            Attachment.reparent_all(self.env, 'wiki', old_name, 'wiki',
    190190                                    new_name)
    191191
    192         self.name = new_name
     192        self.resource.id = self.name = new_name
    193193        self.env.log.info('Renamed page %s to %s', old_name, new_name)
    194194       
    195195        for listener in WikiSystem(self.env).change_listeners:

I propose to fix it in stable and old-stable too.

Attachments (1)

20130328_add-test_11138.patch (608 bytes ) - added by Steffen Hoffmann 6 years ago.
add assertion to ensure correct page.resource.id after rename

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by Steffen Hoffmann

May I provide a test case too, or is it too trivial?

comment:2 Changed 6 years ago by Remy Blank

Thanks for the patch. A test case would be nice, or if there's already a test case for WikiPage.rename(), add an assertion for the resource attribute there.

Last edited 6 years ago by Remy Blank (previous) (diff)

comment:3 Changed 6 years ago by Remy Blank

Milestone: 0.12.6
Owner: set to Remy Blank
Status: newassigned

We indeed do have a test case (test_rename_page), so you can just add an assertion after the one checking page.name.

comment:4 Changed 6 years ago by Christian Boos

Good catch Steffen!

Changed 6 years ago by Steffen Hoffmann

add assertion to ensure correct page.resource.id after rename

comment:5 in reply to:  3 Changed 6 years ago by Steffen Hoffmann

Replying to rblank:

We indeed do have a test case (test_rename_page), so you can just add an assertion after the one checking page.name.

Done here.

Replying to cboos:

Good catch Steffen!

You're welcome. Piece of luck, improving both, a plugin and Trac core at once.

comment:6 Changed 6 years ago by Remy Blank

Resolution: fixed
Status: assignedclosed

Applied in [11735].

comment:7 Changed 6 years ago by Remy Blank

Owner: changed from Remy Blank to Steffen Hoffmann

comment:8 Changed 5 years ago by Ryan J Ollos

Release Notes: modified (diff)

comment:9 Changed 4 years ago by Ryan J Ollos

#11648 proposes changes that would implicitly avoid defects like the one seen in this ticket. Comments on the proposed changes are appreciated.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted.
to The owner will be changed from Steffen Hoffmann to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.