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: | 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
33 33 34 34 <py:def function="create_ticket(teo=False)"> 35 35 <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" 38 38 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"> 41 41 ==== How to Reproduce ==== 42 42 43 43 While doing a $req.method operation on `$req.path_info`, Trac issued an internal error. -
trac/wiki/tests/wikisyntax.py
277 277 </p> 278 278 ------------------------------ 279 279 ============================== Relative to the base url 280 [/newticket? priority=high bug]280 [/newticket?field_priority=high bug] 281 281 [/ Project] 282 282 ------------------------------ 283 283 <p> 284 <a href="/trac/newticket? priority=high">bug</a>284 <a href="/trac/newticket?field_priority=high">bug</a> 285 285 <a href="/trac">Project</a> 286 286 </p> 287 287 ------------------------------
Probably, TracTickets#PresetValuesforNewTickets requires the update, also.
Attachments (2)
Change History (26)
comment:1 by , 18 years ago
Cc: | added |
---|
comment:2 by , 18 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 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 , 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 , 18 years ago
Severity: | critical → blocker |
---|
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 , 18 years ago
Milestone: | 0.11.1 → 0.11 |
---|
comment:7 by , 18 years ago
(also don't forget to adapt the ticket clone code in the template to the 0.10 convention)
comment:8 by , 17 years ago
Priority: | normal → highest |
---|
comment:11 by , 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 , 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:14 by , 17 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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 , 17 years ago
Priority: | highest → low |
---|---|
Severity: | blocker → minor |
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.
follow-up: 17 comment:16 by , 17 years ago
Component: | general → ticket system |
---|---|
Priority: | low → high |
Severity: | minor → blocker |
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.
comment:17 by , 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 , 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 , 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 , 17 years ago
Don't modify req.args
in match_request
comment:20 by , 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.
follow-up: 23 comment:21 by , 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 , 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.
comment:23 by , 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 , 17 years ago
Attachment: | fix_populate_fields-r6787.diff added |
---|
Try another approach, without touching to the req.args
comment:24 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
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).