Closed Bug 103568 Opened 23 years ago Closed 14 years ago

"Keyword" "changed (blah)" Boolean Chart does not work.


(Bugzilla :: Query/Bug List, defect)

Not set





(Reporter: asa, Unassigned)



(Whiteboard: [needs new patch])


(1 file, 6 obsolete files)

Keyword | Changed by/before/after | <user name>/<date>

does not work. The result is a page that says "Unknown keyword named <user name>
<date>.  The legal keyword names are listed here."

There is no way to query for keyword changes. Changed by/before/after other
fields like BugsThisDependsOn and OtherBugsDependingOnThis works just fine.

Tested on b.m.o (2.15)
Blocks: 86547
Blocks: 103885
No longer blocks: 103885
Bug 81642:  As mentioned before, not a blocker.
Bug 100490: Already fixed.
Bug 102618: Not a blocker, because a simple workaround exists.
Bug 95722:  Looks cool, not a blocker.
QuickSearch (bugs 76484, 37339, 102691, 101515):
            Too many bugs.  See note below about limiting nominations.
Bug 98801:  I'd like to see this make the upgrade, but it may
            require more baking than we have time for in this
            installation cycle.
Bug 91486:  Already fixed.
Bug 103568: There does not seem to be any action on this one.

>Finally, you may already have discovered that I've been partly (mis)using this
>bug to submit my RFAs ("requests for attention" ;-).

Yes, I have discovered that.  Stop it.  Misusing this bug in the way 
you describe has no positive impact on your nominations and a negative
impact on your reputation, which in the long run has a negative impact
on your influence in the project.

Use the IRC channel and newsgroup to advocate for bug fixes you care about.
Nominate bugs as blockers for the upgrade only if their effects are severe
and debilitating or their enhancements significantly increase usability
*and* they have good fixes that are almost ready to go.

Ignore the last comment, I posted to the wrong bug.
Asa, I'll take this one. I think I know what the matter is. We're validating the
$value against a bug_id without taking into consideration what the boolean
operator is (changed_by, for instance). So this currently only seems to work for
Assignee: endico → kiko
Okay, seems like a very simple fix. I don't know how the boolean chart works
very well but this has been tested rather extensively on and I
am sure it WFM(tm).

I'll add some keywords to some bugs so you can query for changedby 

is the place you want to be.
Keywords: patch, review
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.16
Comment on attachment 53750 [details] [diff] [review]
kiko_v1: add a check to keywords sub to avoid querying for keywords when when $value isn't a keyword

What about "any words", "all words" and "none of the words"? Do they make no sense for the keywords field? Or do they do what I expect (in which case, we need to validate the input there too)?

Attachment #53750 - Flags: review+
kiko: ping?

Comment on attachment 53750 [details] [diff] [review]
kiko_v1: add a check to keywords sub to avoid querying for keywords when when $value isn't a keyword

>          "^keywords," => sub {
>              GetVersionTable();
>+             if ( ! $t || ( $t ne "equals" && $t ne "notequals" &&
>+                    $t ne "changedto" ) ) {
>+                 # If we aren't doing equal to, no point in looking for
>+                 # and validating keyword names.
>+                 return;
>+             }

Do we need to get the version table if we are not going to validate keyword names?

Further down in the code we check to see if the operator ($t) is "anywords" or "allwords":

                 if ($t eq "anywords") {
                     $term = $haveawordterm;
                 } elsif ($t eq "allwords") {
                     $ref = $funcsbykey{",$t"};
                     if ($term && $haveawordterm) {
                         $term = "(($term) AND $haveawordterm)";

With this patch, this code will never get run, because the function will return
before reaching it if the operator is not either "equals", "notequals" or "changedto".
If we don't need this code anymore, then it should be removed.  Otherwise it needs
to be run as necessary.
Attachment #53750 - Flags: review-
Ugh. Sorry for being absent. I'm very very busy till december. :/

My patch does look broken, but it works, so I'll have to investigate. I'm not
sure if we need the versiontable or not (afterall, I know _nothing_ about this
page at this point) but I think we're not supposed to be returning there. Will
look into it as soon as my local bugzilla works - next monday or tuesday.
We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Hmmm, I'm overburdened at the moment, and can't tackle this one. Any
Assignee: kiko → nobody
Keywords: patch, review
I have a fix for this.
Assignee: nobody → myk
Attached patch patch v2: fixes problem (obsolete) — Splinter Review
This patch simplifies the logic of how buglist.cgi handles keywords quite a bit
while also fixing the problem.	I have tested it in a variety of ways and
cannot figure out why the logic was more complex in the first place.  Perhaps
Terry or more testing can shed some light on this.

In any case, this patch fixes the problem without any noticable side-effects. 
Ready for review.
Attachment #53750 - Attachment is obsolete: true
Adding Terry to the cc: list.  Perhaps he can shed some light on any problems
these changes may cause.
Its complicated because equals != contains, although the keyword cache sort of
wipes away most of thtat. Maybe that cache wasn't there when this code was written?
This stuff has never used the keyword cache.
Comment on attachment 63757 [details] [diff] [review]
patch v2: fixes problem

You store the keyword ids in @keywordids, but then never actually use that (or
set $term), so seraching on keywords won't work.

I'm not sure why this does ((term1) AND (term1 AND term2)) either, but it looks
odd enough to be wrong

Maybe you should use a key of "^keywords,(anywords|allwords|....)" 

instead of the grep?
Attachment #63757 - Flags: review-
any update on Myk's patch?
Attached patch patch v3: unrotted, less cruft (obsolete) — Splinter Review
Here's a patch for the tip.  This goes all the way to using the keyword cache,
removing any leftover cruft from the previous version like keyword ID
collection and term definition.  It doesn't fix everything.  In particular,
string searches have edge cases (f.e. searching for "o b" on a system with
"foo" and "bar" keywords), and "changed(from|to)" won't find changes where the
changed string contains multiple keywords.  The first issue is low priority,
and I've added a comment to the code explaining how to fix the second issue.
Attachment #63757 - Attachment is obsolete: true
This version fixes changedfrom and changedto as well
Attachment #119005 - Attachment is obsolete: true
This patch escapes meta-characters in the string so MySQL doesn't interpret
Attachment #119013 - Attachment is obsolete: true
Attachment #119014 - Attachment is obsolete: true
Attachment #119018 - Flags: review?(justdave)
Unrotted version of last patch.  I also remove unnecessary checks for
"changedfrom" and "changedto" from the standard "keywords" function since there
are custom functions for those operators.
Attachment #119018 - Attachment is obsolete: true
Attachment #119018 - Flags: review?(justdave)
Comment on attachment 128993 [details] [diff] [review]
patch v7: unrotted

Dave, can you try out this latest version of the patch?
Attachment #128993 - Flags: review?(justdave)
Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Comment on attachment 128993 [details] [diff] [review]
patch v7: unrotted

This will have the a problem similar to the one that caused bug 244650

The keywords search will need to be a LEFT JOIN otherwise searches for (keyword
criteria) OR (something non-keyword-related) will skip any records containing
no keyword at all.
Attachment #128993 - Flags: review?(justdave) → review-
This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.
Target Milestone: Bugzilla 2.20 → ---
QA Contact: mattyt-bugzilla → default-qa
Blocks: 351446
Assignee: myk → query-and-buglist
Severity: normal → minor
OS: Windows 2000 → All
Priority: P2 → --
Hardware: x86 → All
Summary: boolean advanced query for keyword changed is broken → "Keyword" "changed (blah)" Boolean Chart does not work.
Whiteboard: [needs new patch]
WFM in Bugzilla 3.4 and newer.
Closed: 14 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.