Opened 14 years ago

Closed 14 years ago

#260 closed defect (fixed)

bug in traces page

Reported by: steve@… Owned by: steve@…
Priority: major Milestone:
Component: admin Version:
Keywords: Cc:

Description

I fail to mark all my traces as public. I have 88 traces that are listed 20 per page. When I go to page 3 and mark all traces (41-60 of 88) as public, all other (1-40 and 61-88) become private.

This is due to a trivial logic error in the SVN source file www.openstreetmap.org/eruby/upload-gpx.rhtml which I could fix if this was C, Java or Perl, but I simply lack experience in the details of Ruby to hack this myself.

Here is the current code, starting on line 72:

if loggedin

if cgiaction? == 'update'

dao.gpx_ids_for_user(user_id).each_hash do |row|

id = rowid? rm = cgirm_? == 'on' priv = cgipriv_? == 'on' if rm

dao.schedule_gpx_delete(id.to_i) %> <i>The gpx file <%= rowname? %> has been scheduled for deletion.</i><

br>

<%

end dao.gpx_set_private(id.to_i, !priv) if dao.does_user_own_gpx?(user_id, id.to_i)

end

end

At first, it seems backwards to loop over all traces belonging to that user. One would think the script should loop over the traces mentioned in the CGI call. However, that could be a security risk when users try to modify traces belonging to others and the current design isn't all that bad. The handling of deletes is also fully OK. If the loop is changed, it would have to call "dao.does_user_own_gpx?" for every trace.

The current problem is with "priv". First the name is bad, since the "on" value indicates publicity and not privacy, which is reflected in the !priv argument to the dao function. The variable should perhaps be named "publ" so that "!publ" is the argument.

But the real bug is that this is kept "on" (public) only if the trace was mentioned in this CGI call. All other traces (in my case 1-40 and 61-88) will be marked "off" (private). The dao.gpx_set_private() function that modifies the priv/publ status, should be called *only* if the variable was present among the current CGI variables. The test "if dao.does_user_own_gpx?" that follows is designed for the case that we loop over the present CGI variables. Perhaps the loop should be altered after all?

As I think you all agree, this is a very long description of a very trivial bug. I have not entered it into trac, because I don't know how to write it any shorter. I hope someone else can take it from here.

Change History (1)

comment:1 Changed 14 years ago by richard@…

Resolution: fixed
Status: newclosed

Fixed.

Note: See TracTickets for help on using tickets.