Edgewall Software
Modify

Opened 18 years ago

Closed 17 years ago

#5025 closed defect (fixed)

[patch] New ticket details are not passed from Trac error pages

Reported by: pkou@… Owned by: Christian Boos
Priority: high Milestone: 0.11
Component: ticket system Version: devel
Severity: blocker Keywords:
Cc: s.lipnevich@…, Noah Kantrowitz Branch:
Release Notes:
API Changes:
Internal Changes:

Description

This is a follow-up to #2945, r4347, r4387

When new ticket is created, it can populate fields from the request. However, formatting rules are changed: instead of name=value it is necessary to use field_name=value.

This change has not been applied to all places.

Fix:

  • trac/templates/error.html

     
    3333
    3434  <py:def function="create_ticket(teo=False)">
    3535    <input type="hidden" name="preview" value="1" />
    36     <input type="hidden" name="reporter" value="${get_reporter_id(req)}" />
    37     <input py:if="teo" type="hidden" name="version"
     36    <input type="hidden" name="field_reporter" value="${get_reporter_id(req)}" />
     37    <input py:if="teo" type="hidden" name="field_version"
    3838           value="${'dev' in trac.version and 'devel' or trac.version}" />
    39     <input type="hidden" name="summary" value="$message" />
    40     <textarea name="description">
     39    <input type="hidden" name="field_summary" value="$message" />
     40    <textarea name="field_description">
    4141==== How to Reproduce ====
    4242
    4343While doing a $req.method operation on `$req.path_info`, Trac issued an internal error.
  • trac/wiki/tests/wikisyntax.py

     
    277277</p>
    278278------------------------------
    279279============================== Relative to the base url
    280 [/newticket?priority=high bug]
     280[/newticket?field_priority=high bug]
    281281[/ Project]
    282282------------------------------
    283283<p>
    284 <a href="/trac/newticket?priority=high">bug</a>
     284<a href="/trac/newticket?field_priority=high">bug</a>
    285285<a href="/trac">Project</a>
    286286</p>
    287287------------------------------

Probably, TracTickets#PresetValuesforNewTickets requires the update, also.

Attachments (2)

move-support-for-new-ticket-URLs-in-process_newticket_request-r6692.diff (3.2 KB ) - added by Christian Boos 17 years ago.
Don't modify req.args in match_request
fix_populate_fields-r6787.diff (3.5 KB ) - added by Christian Boos 17 years ago.
Try another approach, without touching to the req.args

Download all attachments as: .zip

Change History (26)

comment:1 by s.lipnevich@…, 18 years ago

Cc: s.lipnevich@… added

comment:2 by Christian Boos, 18 years ago

Owner: changed from Jonas Borgström to Christian Boos
Status: newassigned

I think we can apply the patch for consistency, even it's not completely settled at this point if we should keep that naming (see #4947).

comment:3 by pkou at ua.fm, 18 years ago

It seems that the proposed naming in #4947 (rename version to revision) is better than having field_xxx for ticket fields.

comment:4 by Christian Boos, 18 years ago

Thinking a bit more about this, I believe the /newticket and /ticket requests don't necessarily have to have the same conventions.

Spelling out /newticket URLs is quite common usage, and being able to use directly the ticket fields without a prefix is convenient. Also, there's no need for a version parameter here, or a format one or any other kind of special parameter.

OTOH, when passing parameters to ticket URLs or links, what we expect to use are the special parameters like version or format. Being able to specify a ticket field has little if any use there, so it's OK if a field_ prefix is expected for field parameters in this situation.

comment:5 by Christian Boos, 18 years ago

Severity: criticalblocker

t.e.o should not be upgrade to 0.11 until the /newticket processing gets fixed like exlained above, otherwise creating semi-automated bug reports won't work.

comment:6 by Christian Boos, 18 years ago

Milestone: 0.11.10.11

comment:7 by Christian Boos, 18 years ago

(also don't forget to adapt the ticket clone code in the template to the 0.10 convention)

comment:8 by anonymous, 17 years ago

Priority: normalhighest

comment:9 by ThurnerRupert, 17 years ago

is there a special reason why this does not get applied?

comment:10 by anonymous, 17 years ago

Any status to report? This appears to be the critical path item.

comment:11 by anonymous, 17 years ago

Agree that more information would be helpful to understand. Can all / some of this be deferred in the interested of releasing 0.11?

comment:12 by Christian Boos, 17 years ago

This ticket needs to be resolved before we upgrade t.e.o to 0.11dev, as otherwise the semi-automatic bug reporting mechanism will not work. The way to fix it is described above in comment:4, so fixing the ticket is only a matter of time.

comment:13 by Christian Boos, 17 years ago

Resolution: fixed
Status: assignedclosed

Fixed in r6120.

comment:14 by Noah Kantrowitz, 17 years ago

Cc: Noah Kantrowitz added
Resolution: fixed
Status: closedreopened

This is not an acceptable fix. All thing related to the old field_ names should be removed rather than just changing the request magically.

comment:15 by Christian Boos, 17 years ago

Priority: highestlow
Severity: blockerminor

What are you suggesting exactly? Removing the field_ prefix in the templates?

Also, can you elaborate why the fix is not "acceptable" according to you? The way you put it doesn't look very constructive. I'd suggest you first try to find a better approach, come with a patch, then we can discuss it.

comment:16 by Noah Kantrowitz, 17 years ago

Component: generalticket system
Priority: lowhigh
Severity: minorblocker

This is not acceptable because there are other plugins that don't appreciate having their request args changed behind the scenes. Specifically this makes the template debugger unusable. This is very much a blocker issue, it is an incredibly obtrusive mechanism. If we are not using the field_ prefix, it needs to go away entirely not just hidden.

in reply to:  16 comment:17 by Christian Boos, 17 years ago

Replying to nkantrowitz:

… If we are not using the field_ prefix, it needs to go away entirely not just hidden.

Well, not at all, the field_ prefix is used, of course. I think you got it backwards, we're not hidding the field_ prefix, we're adding it when it's not there in very specific circumstances.

The only situation where a parameter mapping will take place is for GET requests to /newticket, as the only thing that will trigger such requests are hand-written URLs or TracLinks (like [/newticket?component=thing file a bug for the thing]). See r6120 / r6122.

So for newticket POSTs and ticket POSTs that happen during normal use, there's no parameter change under the hood.

There are two alternatives for not having this special case:

  • not having the field_ prefix at all - this is the 0.10 situation, and the problem with that approach, besides the conflict of meaning between version (field or ticket version?) is that it's error prone as custom fields may clash with any "reserved" parameter
  • having the field_ prefix everywhere, including for /newticket URLs. This has the disadvantage of invalidating all pre-existing hand-written URL, and is also a bit cumbersome to write

Both have disadvantages, so that's why I came up with the middle ground approach (comment:4) which at that time sounded OK (IIRC cmlenz reviewed the patch). And that's why I asked (genuinely) if you had a better idea.

comment:18 by Christian Boos, 17 years ago

Taking into account the constraints exposed above, I see one solution which could work: in the /newticket match_request, only rename the parameters matching actual field names, leaving the others alone.

Would that be enough for the plugins?

comment:19 by Noah Kantrowitz, 17 years ago

The easiest way to handle this will probably be to handle the translation inside process_request, and also to make a new dict with the modified arguments instead of destructively changing req.args. This should at least prevent the current insanity from leaking out to the world.

by Christian Boos, 17 years ago

Don't modify req.args in match_request

comment:20 by Christian Boos, 17 years ago

Please have a look at the patch. I didn't follow your suggestion to make a new dict, as req.args was already used throughout the whole Ticket module and passing that new dict around would have required too many changes.

comment:21 by Noah Kantrowitz, 17 years ago

This still changes req.args, so I don't know why you think this is any better. Pre-request filters are okay now, but post-request and stream filters are both in the same situation. Just do a s/req.args/args/, on that file or something, or find a real way to fix this rather than just moving the problem around.

comment:22 by ebray <hyugaricdeau@…>, 17 years ago

Right, this would still allow allow custom fields to clobber reserved names. I preferred the other way better, though nkantrowitz has valid concerns.

What about just leaving everything alone for the common case (build-in fields), so don't mess with req.args at all. And for custom fields (less common) require something annoying, like that they be prefixed with an underscore (for example).

I have some users who would be confused by this, but maybe a reasonable middle ground would be something like that.

in reply to:  21 comment:23 by Christian Boos, 17 years ago

Replying to nkantrowitz:

This still changes req.args, so I don't know why you think this is any better.

Because only req.args entries corresponding to field names are normalized, any others are left unchanged.

Pre-request filters are okay now, but post-request and stream filters are both in the same situation.

Well, I'm not sure why you think it's better for post-request and stream filters to see the original parameters instead of the normalized ones. Let's say there is a reason, and we make it as a rule that process_request should not alter the req.args - but please note that was not the case so far (e.g. the del req.args['field_owner'] in _process_newticket_request). Also, simply adding to the req.args is even common practice in most of the match_request.

Just do a s/req.args/args/, on that file or something, or find a real way to fix this rather than just moving the problem around.

Ok, so we would definitely need to do that if we want to preserve req.args, not only for the problem discussed here, but also for the #3295 case (where the del req.args['owner'] change was originally introduced).

by Christian Boos, 17 years ago

Try another approach, without touching to the req.args

comment:24 by Christian Boos, 17 years ago

Resolution: fixed
Status: reopenedclosed

Above patch committed in r6834. There's also r6835, which re-introduces a check for /newticket?id=xyz (#5022), which is again needed.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos 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.