Topic version bumps up without change if you call the save script alone. While no big issue, it changes the last author which can be problematic based on the context.

Don't laugh, this happens not too unfrequently.

• someone makes some changes, previews the topic (no save)
• decides to send an e-mail with the url to a coworker, it looks like this: https://mail.twiki.org/do/save/Bugs/Item1325
• the recepient clicks on the link

Cairo had a protection against this scenario by preventing topic save if there is no data.

Do I get a TWiki heart award or an onion for filing so many bugs?

-- PTh

I changed your example URL to what I think you meant. I assume you are concerned that the recipient of the email will hit save?

Cairo's "protection" was reported as a bug several times (cannot save an empty topic). It seems to me that the action of calling "save" needs to be treated as exactly that; a "save". Otherwise we are faced with yet another special case that solves this one example use case but breaks a whole bunch of others.

A more appropriate solution to the scenario you picture above would be to disable the save button on the preview screen if there is no 'text' URL parameter. This can be done using %IF{"defined text" then="enabled" else="disabled"}% (try it: click please do not press this button). Split into Item1326 and Item1327.

No heart or onion yet; you have a long way to go to catch up with Anton in the bug reporting stakes!

• Regex search: -- PTh
  • META.*ReportedBy.*AntonAy - 76
  • META.*ReportedBy.*CrawfordCu - 116
  • META.*ReportedBy.*PeterTh - 128

CC

No, I meant save, not preview since the preview screen is delivered by the save script. Try it out. So sending a preview URL is actually a save for the recepient. This needs to be protected against in the code.

-- PTh

the same issue applies to any other save / commit url that someone might paste, and none are truly able to be protected against.. simple eg is to make a direct save url - it will also always save... as we have revision control, and you can never loose a revision, its more consistent to make save do exactly that.

its not simple really

SD

My point still applies, whether it is save or preview that generates the screen. It should be handled in the template.

CC

No, not like this. Please follow the scenario I described. The second user never sees a save button, simply clicking on the link will bump up the version.

The solution is actually simple: Prevent save if the topic text is empty (or the parameter is missing).

-- PTh

preventing save if the parameter is missing would work; however, preventing save on an empty parameter prevents saving an empty page. i thought that's what i had coded...

-- WN

The (convoluted) logic of the save script is as follows:

1. If the action_ parameter is set, perform that action or
2. If text is defined, save that text or
3. If text is not defined, but templatetopic is defined, use the text from that topic as the new text or
4. If the topic already exists, read the text from the topic or
5. the text is the empty string.

From this logic, it is clear that the case of a URL with no parameters will trigger case 4 i.e. the topic will be re-saved with the existing text. The idea behind case 4 is that when a save is simply changing the form, or the topic parent, or some other similar hack, the text is left untouched. In the event that a save with empty text is being done, the code automatically enables forcenewrevision to ensure that nothing is lost. Thus the version of the topic gets incremented.

Doing what Peter suggests, and breaking the empty-topic bugfix again, is unacceptable.

Here's what I think is the correct fix. If case 4 is hit but none of the relevant parameters to trigger the non-text change (any of formtemplate, forcenewrevision, topicparent) are defined, it is an error state and the script can be aborted (an OopsException thrown).

However, when we discussed this before we elected to simply force a new revision if the text is empty, so there is no risk of losing content. If someone feels strongly about this, they can implement (and test!) what I describe above. However I consider it to be too high a risk for me, as I haven't got time to test it thoroughly, so I am setting this to "deferred".

CC

We are not in agreement on preventing empty topic save, but I can live with it. So lets have it your way.

But I do not think that a simple check if there are any parameters early on in save is a risky change since save is never called without parameters. It is probably a one line change, something like:

unless( $query->param ) { complain( $url ) };

So we should take this into Dakar.

-- PTh

Sorry, no. That hack covers one special case, but still needs just as much testing as the correct fix. And to defeat it, all it needs is ?foobar= on the URL. Check the parameters properly, please don't just try to hide flawed logic.

I will leave this as "actioning" instead of "deferred" so someone else can come along and fix it; I don't have time.

CC

This is a usability issue. The fix to check for any parameter guards against the common scenario described above, and it is a low risk change.

Is anyone helping out? If not I will take it on my to-do list (which unfortunally just extends the critical path.)

-- PTh

SVN 8190 fixes it properly.

CC

Excellent, thanks Crawford

Small suggestion for improvement, this is to address the scenario described above:

A user receiving the e-mail with the incorrect URL is not so much interested in finding out what the correct parameters to the save script are, (s)he would like to see the topic. Therefore, in addition to the save script help text it would be helpful to have a link back to the topic, somethig like:

[[%WEB%.%TOPIC%][Cancel save]] and go back to <nop>%TOPIC%.

-- PTh

I considered that, but rejected it. There are at least two scenarios in which the save script is likely to be called without parameters.

The first is, as you have described, when a preview URL is erroneously posted and clicked in the browser.

The second is when the save script is invoked from a shell script e.g. bulk upload, or offline update. In this second scenario, the only way to determine the error status is to check the response from the script, something like this (example from BuildContrib)

    my $url = "http://twiki.org/cgi-bin/save/$web/$topic";
   $response = $userAgent->post( $url, \%newform );

    die 'Update of topic failed ', $response->request->uri,
      ' -- ', $response->status_line, 'Aborting'
        unless $response->is_redirect &&
          $response->headers->header('Location') =~ /view([\.\w]*)\/$web\/$topic/;

Note that the redirect to the view script is the only way to check the result of the save. If an empty form were to result in a redirect to view, that would give a false positive to the script.

CC

Lets please make this user centric. I am not suggesting to remove the oops message, just to add the above mentioned text to the oops message. I do not understand how this additional text would affect the automated script.

-- PTh

Oops, sorry, I didn't parse your comment correctly; I though you meant to redirect to the view script instead of oops.

The only possible reason for not adding the text to the message is that the message is a generic one and may be used in many places. But that's a weak reason. Please feel free to amend the message if you want.

CC

Fair enough. Recalssifying as Urgent so that it does not get forgotten.

-- PTh

Nice example of a constructive dialogue against all odds

SVN 8255, please re-phrase if necessary.

-- SP

Well, thank goodness for testcases! Peter was actually right, but for the wrong reasons

Checking the parameters in the way I implemented has a problem; I specifically recoded the save script so you could save a single form field value in a url. Because formfield values can be just about anything, there is no way to check the parameters; the best you can do is fail if there are no parameters!

If I had run the unit tests I would have realised

CC