Reporter: steve[at]fractalus.com
[Submitted to the original trac issue database at 6.40pm, Monday, 24th July 2006]

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 cgi['action'] == 'update'
dao.gpx_ids_for_user(user_id).each_hash do |row|
id = row['id']
rm = cgi["rm_#{id}"] == 'on'
priv = cgi["priv_#{id}"] == 'on'
if rm
dao.schedule_gpx_delete(id.to_i)
%>
The gpx file <%= row['name'] %> has been scheduled for deletion.<
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.