If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Advanced Query: Use JavaScript to modify/create boolean charts

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Query/Bug List
P1
enhancement
RESOLVED FIXED
18 years ago
6 years ago

People

(Reporter: CodeMachine, Assigned: Max Kanat-Alexander)

Tracking

2.13
Bugzilla 4.2

Details

(Whiteboard: [fixed by blocker])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

18 years ago
I'm well aware Terry's policy was not to require Javascript for users of
Bugzilla - and the advanced querying facility fits in with this.

But it would be nice if Javascript was used to alter the page instead, where
available.  This would be much quicker that loading the page again.

Not sure what DOM level this would require (1?), but I'm pretty sure it's
possible.  I guess implementation would involve a script that altered the button
behaviour where the DOM facilities are present.
(Reporter)

Updated

17 years ago
Whiteboard: Future-Target

Comment 1

17 years ago
moving to real milestones...
Target Milestone: --- → Future
(Reporter)

Comment 2

16 years ago
Moving to new Bugzilla product ...
Assignee: tara → endico
Component: Bugzilla → Query/Bug List
Product: Webtools → Bugzilla
Whiteboard: Future-Target
Version: other → 2.13
Assignee: endico → nobody
Unless I misunderstand what this bug is about, this WORKSFORME by now.

Comment 4

12 years ago
Created attachment 214220 [details] [diff] [review]
call new javascript

First part of patch: link new javascript page into the boolean_chart section of the search page, and modify event handlers.
Attachment #214220 - Flags: review?(myk)

Comment 5

12 years ago
Created attachment 214221 [details]
javascript to modify boolean charts client side

This javascript file contains code to alter the boolean charts client side.
Attachment #214221 - Flags: review?(myk)
(Assignee)

Comment 6

12 years ago
Okay, now that we have an actual patch, let's make this bug's summary more specific.
Assignee: nobody → remi_zara
Summary: Use Javascript where possible in advanced querying. → Advanced Query: Use JavaScript to modify/create boolean charts
Target Milestone: Future → Bugzilla 2.24
Comment on attachment 214220 [details] [diff] [review]
call new javascript

Looks good, r=myk
Attachment #214220 - Flags: review?(myk) → review+
Comment on attachment 214221 [details]
javascript to modify boolean charts client side

>function alterBooleanChart(evt) {
>    evt = evt ? evt: window.event;

Nit: put a space before the colon, i.e.: evt: -> evt :


>    if (!document.getElementById) {
>        // not so much dom support
>        document.forms[0].action='query.cgi#chart';
>        document.forms[0].method='POST';
>        return 1;
>    }

Nit: I know the original code returned "1", but I think its intent is to return boolean "true", and since JavaScript (unlike Perl) actually has boolean primitives, and since you're rewriting the code anyway, use "true" here instead of "1".


>    // get the ids of the new chart elem to create
>    var addCmdIds = /cmd\-add(\d+)-(\d+)-(\d+)/.exec(evt.target.name).slice(1);
>    if (addCmdIds[2] != "0") {
>        // add a chart column (Or)
>        addChartCol(evt,addCmdIds);
>    } else if (addCmdIds[1] != "0") {
>        // add a chart row (And)
>        addChartRow(evt,addCmdIds);
>    } else {
>        // add a new chart
>        addChart(evt,addCmdIds);
>    }
>    // switch to POST method
>    document.getElementsByName("queryform")[0].method = "POST";
>    // prevent default action
>    evt.preventDefault();
>    evt.stopPropagation();
>    return false;
>}

Nit: capitalize "Id", i.e. addCmdIds -> addCmdIDs.
Nit: it would make this code more readable to add blank lines between logical blocks and the last return statement, i.e.:

    // get the ids of the new chart elem to create
    var addCmdIds = /cmd\-add(\d+)-(\d+)-(\d+)/.exec(evt.target.name).slice(1);
    if (addCmdIds[2] != "0") {
        // add a chart column (Or)
        addChartCol(evt,addCmdIds);
    } else if (addCmdIds[1] != "0") {
        // add a chart row (And)
        addChartRow(evt,addCmdIds);
    } else {
        // add a new chart
        addChart(evt,addCmdIds);
    }

    // switch to POST method
    document.getElementsByName("queryform")[0].method = "POST";

    // prevent default action
    evt.preventDefault();
    evt.stopPropagation();

    return false;
}


>/*
> * Add a new chart column (Or)
> * @param evt event on the add button
> * @param addCmdIds ids of the new chart elem to create
> */
>function  addChartCol(evt,addCmdIds) {

Nit: put spaces after commas, i.e.: evt, addCmdIds.


>    var negateInput = document.getElementById("negate"+addCmdIds[0])

Nit: put spaces around plus signs, i.e. "negate" + addCmdIds[0]


>    if (negateInput) {
>        // already exists
>        return;
>    }

Nit: omit brackets around one-line conditional and looping blocks, i.e.:

if (negateInput)
    return


Otherwise this code looks good, but while testing I noticed a regression in functionality: it's no longer possible to remove elements from boolean charts.  Without JavaScript, you can do so by going "Back" in your browser's history, but with JavaScript enabled you can no longer do that.  You should add a button to each row that removes the row from its chart so that we don't regress the functionality of boolean charts.
Attachment #214221 - Flags: review?(myk) → review-

Updated

11 years ago
QA Contact: mattyt-bugzilla → default-qa

Comment 9

11 years ago
This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
(Assignee)

Comment 10

10 years ago
Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.
Target Milestone: Bugzilla 3.2 → ---

Comment 11

9 years ago
pyrzak, I'm sure that's something you want to do. ;) This would avoid useless calls to the server when all we want is a new set of select fields.
Priority: P3 → P1
Target Milestone: --- → Bugzilla 4.0

Comment 12

9 years ago
Yeah this looks like a good one. I'll make sure to add it to the 4.0 bug list.
A way to remove a row would be good while we're at it. (right now once you've added a row there's now way to get rid of it).
(Assignee)

Comment 14

8 years ago
Seriously, somebody should finish this. :-)
Assignee: remi_zara → query-and-buglist
Whiteboard: [needs new patch]
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6

Comment 15

8 years ago
We might be doing something like this at NASA and i should be able to port it over once I've got a design from our NASA designers. I'm sure JS for the boolean charts will be part of it.

Comment 16

8 years ago
it appears that currently boolean charts are not supported without JS. Therefore the new implementation in JS will not attempt to maintain any functionality beyond 1 boolean chart without JS being turned on. Please post if this is incorrect. Tested in FF3.5 on a mac

Comment 17

8 years ago
Created attachment 408332 [details] [diff] [review]
Javascript Boolean Charts

allows for adding, removing and caching of dynamically added boolean charts.
Attachment #214220 - Attachment is obsolete: true
Attachment #214221 - Attachment is obsolete: true
Attachment #408332 - Flags: review?(mkanat)
Attachment #408332 - Flags: review?(LpSolit)
(Assignee)

Comment 18

8 years ago
Comment on attachment 408332 [details] [diff] [review]
Javascript Boolean Charts

This is really cool! :-)

I haven't looked at the code yet, but here's a few comments:

1) The remove buttons don't seem to work quite right--Remove Chart removed my And, Remove And did nothing, and Remove Or submitted my form, for some reason.

2) I'd really like to see "Remove And" appear as an [x] link next to "And", "Remove Or" appear as an [x] link next to the chart row, and "Remove Chart" appear as an [x] next to the <hr> that creates the chart (or somewhere else reasonable).

That serializer thing looks pretty clever. :-)

Wow, I'm surprised that this patch was so big, but I'm so happy that it's done even though it WAS so large! :-)
Attachment #408332 - Flags: review?(mkanat) → review-
(Assignee)

Comment 19

8 years ago
Here's the bug with the Remove buttons:

1) Click Or
2) Click And on the last row
3) Click Add Chart on the last row
4) Try to Remove Chart (doesn't happen)

Now there's some confusion and the other Remove buttons don't work.

Comment 20

8 years ago
Created attachment 408343 [details] [diff] [review]
V2

some bug fixes and replaced buttons for hyperlinks with [x]
Attachment #408332 - Attachment is obsolete: true
Attachment #408343 - Flags: review?(mkanat)
Attachment #408343 - Flags: review?(LpSolit)
Attachment #408332 - Flags: review?(LpSolit)

Comment 21

8 years ago
Created attachment 408344 [details] [diff] [review]
V2.1

I realized i needed to reserialize after i fix the chart numbers
Attachment #408343 - Attachment is obsolete: true
Attachment #408344 - Flags: review?(mkanat)
Attachment #408344 - Flags: review?(LpSolit)
Attachment #408343 - Flags: review?(mkanat)
Attachment #408343 - Flags: review?(LpSolit)

Updated

8 years ago
Assignee: query-and-buglist → guy.pyrzak
Status: NEW → ASSIGNED
Whiteboard: [needs new patch]

Comment 22

8 years ago
Created attachment 408409 [details] [diff] [review]
V3

Made some usability improvements. The page only refreshes now if the user has actually entered a boolean chart beyond the 1 that exists on the page normally. Also the serializer removes blank query parameters in an attempt to shorten the URL.
Attachment #408344 - Attachment is obsolete: true
Attachment #408409 - Flags: review?(mkanat)
Attachment #408409 - Flags: review?(LpSolit)
Attachment #408344 - Flags: review?(mkanat)
Attachment #408344 - Flags: review?(LpSolit)
(Assignee)

Updated

8 years ago
Attachment #408409 - Flags: review?(mkanat) → review-
(Assignee)

Comment 23

8 years ago
Comment on attachment 408409 [details] [diff] [review]
V3

Okay, I think I wasn't totally clear about my requests on where the [x] should go:

When I add a new Or chart, the [x] should go at the start or end of the chart. The first Or chart or any And set or any Chart should not have an [x]. (I can pretty easily clear those values if I want to, myself.)

When I add a new And chart, the word "and" is interposed between charts. The [x] should be next to the And, because that's the clearest point for the user to realize what it will be doing. (When you have an and and then two ors after it, it's very unclear which And will be deleted by the [x] after the "And" *button*.)

When you add another boolean chart, the [x] should go next to the <hr> that appears at the top of the chart. (Otherwise there's the same confusion about the [x] after "Add Another Boolean Chart", which appears on every item.)


Also, I think search-advanced is not the only template where boolean-charts.html.tmpl is used, so you may have to add your js stuff to other places. (And be wary of going over 80 characters for lines of code.)
(Assignee)

Updated

8 years ago
Attachment #408409 - Flags: review?(LpSolit)

Comment 24

8 years ago
Your request implies that a user can remove any boolean chart (or, and etc) from any location. Currently the user can only remove any boolean chart, not and/ors. Doing this would not be difficult but would require more code for this patch. I'd have to "fix" the boolean charts whenever someone removes one of them. Let me know if you want this as part of this patch or as a separate one. The ability to remove any arbitrary AND/OR is a new feature and I was just trying to not have a regression from the current boolean charts which only allow you to remove the last boolean chart.

Good point about more than just advanced search i'll do that.

Comment 25

8 years ago
For now, we should focus on what this bug is about: use JS to create boolean charts. Ability to delete them should go elsewhere.

Comment 26

8 years ago
pyrzak, any chance to attach an updated patch before we freeze?
Whiteboard: [needs new patch asap]
(Assignee)

Comment 27

8 years ago
Yeah, to be clear about this (pyrzak and I discussed this on IRC, and I didn't see this comment until recently because I wasn't CC'ed on this bug), I don't want any new functionality there as far as deleting, I just wanted the deletion links/buttons in a different place. :-)

Comment 28

8 years ago
Too late for 3.6.
Whiteboard: [needs new patch asap]
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Do we have another bug for removing rows filed yet? I didn't find anything....
(Assignee)

Comment 30

7 years ago
  pyrzak: I'm going to *seriously* refactor Search.pm for 4.2, and boolean charts might end up working totally differently, so you might want to put this on the back-burner for a little while.
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2

Comment 31

7 years ago
(In reply to comment #30)
>   pyrzak: I'm going to *seriously* refactor Search.pm for 4.2, and boolean
> charts might end up working totally differently, so you might want to put this
> on the back-burner for a little while.

Are you done with it now? Can pyrzak update his patch?
(Assignee)

Comment 32

7 years ago
I'm not done with it, no. I'm in touch with pyzak about it, so I will for sure let him know when it's a good idea to start again.

Comment 33

7 years ago
(In reply to comment #32)
> I'm not done with it, no. I'm in touch with pyzak about it, so I will for sure
> let him know when it's a good idea to start again.

Ok, but it's also a good idea to let the community know about this. Your private talks are... private, and we have no idea what's going on here. ;)
(Assignee)

Comment 34

7 years ago
Okay. Well, when this bug is ready to be worked on, either myself or pyrzak will comment in it.
(Assignee)

Updated

7 years ago
Depends on: 647649
Whiteboard: [blocker will fix]
(Assignee)

Updated

6 years ago
Assignee: guy.pyrzak → mkanat
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [blocker will fix] → [fixed by blocker]
You need to log in before you can comment on or make changes to this bug.