Edgewall Software

Opened 9 years ago

Last modified 13 months ago

#9612 closed enhancement

`WikiPage` doesn't need the `ipnr` attribute — at Version 29

Reported by: Martin Scharrer <martin@…> Owned by: Ryan J Ollos
Priority: low Milestone: 1.3.1
Component: wiki system Version: 0.12-stable
Severity: minor Keywords: WikiPage model ipnr
Cc: Thijs Triemstra Branch:
Release Notes:
API Changes:

Added upgrade_tables method to DatabaseManager class. Added method to Table class for removing columns from schema: remove_columns.

Description

Just saw this while changing th:WatchlistPlugin from own SQL code to the trac.wiki.model.WikiPage class:

Instances of the WikiPage class do not provide the ipnr as attribute as for all other wiki page values (version, author, etc.). In fact the _fetch method doesn't read this value from the DB.

If this is not by intention this should be fixed for consistency. Some plug-ins probably like to read this information out, e.g. I would have given the users the option to show it in their watchlist.

I would be willing to provide a patch.

Change History (29)

comment:1 by Christian Boos, 9 years ago

Well, why do you need the ipnr? I always thought nobody was actually interested in that and thought that we should even remove it in later releases.

comment:2 by Remy Blank, 9 years ago

We removed the display of that information some time ago in 0.11-stable IIRC.

in reply to:  1 comment:3 by Martin Scharrer <martin@…>, 9 years ago

Replying to cboos:

Well, why do you need the ipnr? I always thought nobody was actually interested in that and thought that we should even remove it in later releases.

As I said, I'm writing a plugin which shows the wiki data to the user as a table and I just wanted to be as comprehensive as possible. If you think about this that way I will just drop it. It's not an important thing for the plugin.

comment:4 by Christian Boos, 9 years ago

Milestone: next-major-0.1X
Summary: `WikiPage` instances lack the `ipnr` attribute`WikiPage` doesn't need the `ipnr` attribute

All clear then. Let's remember to remove the ipnr information from the wiki table, at some later point.

in reply to:  4 comment:5 by Thijs Triemstra, 8 years ago

Cc: Thijs Triemstra added

Replying to cboos:

All clear then. Let's remember to remove the ipnr information from the wiki table, at some later point.

What version do you have in mind?

comment:6 by Christian Boos, 8 years ago

0.14 of course. I don't think we should do any more schema change at this point for 0.13, as we should slowly try to finalize this version.

comment:7 by Thijs Triemstra, 8 years ago

Milestone: next-major-0.1X0.14

comment:8 by Ryan J Ollos, 5 years ago

I was thinking we should add some notices to the documentation about the pending removal: log:rjollos.git:t9612-schedule-ipnr-removal.

comment:9 by Ryan J Ollos, 5 years ago

Changes from comment:8 committed to 1.0-stable in [13512], merged to trunk in [13513].

comment:10 by Ryan J Ollos, 5 years ago

Should we also remove ipnr from the Attachment class?

in reply to:  10 comment:11 by Jun Omae, 5 years ago

Should we also remove ipnr from the Attachment class?

Yeah. We should consistently remove attachment.ipnr if wiki.ipnr will be removed.

comment:12 by Ryan J Ollos, 5 years ago

Milestone: next-dev-1.1.xnext-major-releases

Thanks. I'll add notices to attachment module about deprecation of ipnr and pending removal, as in comment:8. Changing milestone since work will be done in milestone:1.3.1.

comment:13 by Jun Omae, 5 years ago

Related question, should we also remove [trac] show_ip_addresses option in 1.3.1?

comment:14 by Ryan J Ollos, 5 years ago

Yeah, it looks like that should be removed. The only uses I see aside from adding the value to the Chrome data dictionary are:

Side note: I didn't realize it was possible to link to a specific option and highlight it. I guess that works with any wiki word or phrase though. Very cool!

comment:15 by Ryan J Ollos, 5 years ago

Deprecated ipnr attribute of attachment class in [13729:13730]. Deprecated show_ip_addresses in [13731:13732].

comment:16 by Ryan J Ollos, 4 years ago

Milestone: next-major-releases1.3.1

comment:17 by carstenklein@…, 3 years ago

What about a hacked trac and where people upload unwanted attachments or alter wiki pages in ways we cannot imagine? Wouldn't it be nice to at least have the ip address from where the alteration or malicious upload came from?

comment:18 by Ryan J Ollos, 3 years ago

For a publicly-accessible site, you probably want to be running SpamFilter. See also #12398.

For the specific case in which a site was hacked and you want to know the IP address, there are probably better ways to obtain that information such as logging from the web server.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:19 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:20 by Ryan J Ollos, 3 years ago

Proposed changes in log:rjollos.git:t9612.

comment:21 by Christian Boos, 3 years ago

LGTM, + 2 remarks:

  • we should also remove show_ip_addresses in chrome.py
  • db42.py makes me think we could start to introduce a DDL helper functions, like upgrade_table(old_Table, new_Table) and a Table.remove_columns that would create a new Table instance, so that one could write upgrade_table(attachment_table.remove_columns('ipnr'), attachment_table)

in reply to:  21 comment:22 by Ryan J Ollos, 3 years ago

Replying to Christian Boos:

  • we should also remove show_ip_addresses in chrome.py

show_ip_addresses was removed in r14909. That change should probably have been committed with these changes.

  • db42.py makes me think we could start to introduce a DDL helper functions, like upgrade_table(old_Table, new_Table) and a Table.remove_columns that would create a new Table instance, so that one could write upgrade_table(attachment_table.remove_columns('ipnr'), attachment_table)

Similar aim in #11872. I'll look into it.

comment:23 by Ryan J Ollos, 3 years ago

Proposed changes in log:rjollos.git:t9612.2. Some of the changes will be committed to 1.2-stable to address #11872.

comment:24 by Jun Omae, 3 years ago

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

in reply to:  24 ; comment:25 by Ryan J Ollos, 3 years ago

Replying to Jun Omae:

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

Is this only needed if the column with auto_increment has changed, or has been removed? If the column with auto_increment has not changed, then that column will have the same data in the new table as in the old.

in reply to:  25 comment:26 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

Replying to Jun Omae:

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

Is this only needed if the column with auto_increment has changed, or has been removed? If the column with auto_increment has not changed, then that column will have the same data in the new table as in the old.

The upgrade_tables re-create tables. Auto-increment column uses sequence object in PostgreSQL. Then, the sequence object would be re-created and started with 1. Inserting old records wouldn't change the sequence.

See also #8575.

Last edited 3 years ago by Jun Omae (previous) (diff)

comment:27 by Ryan J Ollos, 3 years ago

[7ea7e2d5/rjollos.git] calls update_sequence. I'll propose additional changes after #11872 is implemented.

comment:28 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

Fix for Cannot operate on closed database error committed to 1.0-stable in r15018, merged to 1.2-stable in r15019, merged to trunk in r15020.

DatabaseManager.upgrade_tables committed to 1.2-stable in r15021,r15023, merged to trunk in r15022,r15024.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:29 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)
Note: See TracTickets for help on using tickets.