621216 - Don't call GetQuip() if the user doesn't want quips

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Reed Loden [:reed]]

Attachment: patch - v1

Even if a user doesn't want quips, GetQuip() is called, which does two queries against the database. There's no need for that.

Comment 1

[Max Kanat-Alexander]

Ah, seems reasonable, but maybe it would be better to only do the query inside of the template, somehow. Generally I like to keep preference checks only in the template, if possible.

Comment 2

[Reed Loden [:reed]]

Wouldn't that require doing directly SQL queries from within the template, which seems bad? Quips could be converted into objects, but I have no interest in rewriting all that code for that.

For the record, we do check some preferences from within *.cgi files... specifically, process_bug.cgi, relogin.cgi, and sanitycheck.cgi.

Comment 3

[Frédéric Buclin]

Comment on attachment 499570 [details] [diff] [review]
patch - v1

>=== modified file 'template/en/default/list/list.html.tmpl'

>-  [% IF user.settings.display_quips.value == 'on' %]
>+  [% IF quip.defined %]

Nit: don't use .defined here. If for some reason the quip is an empty string, or "0" (how useful), then there is no need to display it. Otherwise looks good.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #2)
> Wouldn't that require doing directly SQL queries from within the template,
> which seems bad?

  Ah, not really. I just mean doing something like user.get_quip or something.

> For the record, we do check some preferences from within *.cgi files...
> specifically, process_bug.cgi, relogin.cgi, and sanitycheck.cgi.

  Yes, but I want to avoid it as much as possible. Generally preferences and parameters should only control the UI, unless there is absolutely some reason that they must control the backend.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 499570 [details] [diff] [review]
patch - v1

Since this won't make any significant performance difference, I'd rather see the refactored patch that adds a method to some object that gets a quip, as mentioned in the above comments.

Comment 6

[Frédéric Buclin]

(In reply to comment #4)
>   Yes, but I want to avoid it as much as possible.

I don't see the logic behind this. It's absolutely fine to have this code in buglist.cgi. Also, what you want is $user->wants_quip and $user->get_quip or something like that, which is a pretty useless refactor for such a minor thing.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> I don't see the logic behind this.

  All right.

  First, read this: http://ometer.com/free-software-ui.html

  Then think about isolation, modularity, and decoupling. (And I'm pretty sure we all know what those are, since they're the most basic software design principles out there.) How do we best limit the impact that Havoc is talking about, in terms of those software design principles? (And it's not just Havoc talking about it, and it's not just Havoc's statements we need to care about, but he's making some good points specifically about preferences.)

  Then think about customizers.

  Once you've done all that, a good question is, "What's simpler for customizers and future implementors, preferences that control Bugzilla's behavior, or preferences that only control its UI?" What leads to simpler code? What leads to fewer conditional statements in our code? Which adds more complexity to the system, a new object method or a new conditional statement? Do you understand how to measure that complexity? Have you experienced that complexity?

Comment 8

[Frédéric Buclin]

OK, I don't need you to make a whole speech about complexity and modularity. I'm talking about a very specific thing in buglist.cgi, and most of your speech is irrelevant here.

Comment 9

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 10

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v2

Comment 11

[Frédéric Buclin]

Comment on attachment 806162 [details] [diff] [review]
Patch-v2

>=== modified file 'buglist.cgi'

>+if (Bugzilla->user->settings->{'display_quips'}->{'value'} eq 'on') {

Use $user instead of Bugzilla->user.



>=== modified file 'template/en/default/list/list.html.tmpl'

>-  [% IF user.settings.display_quips.value == 'on' %]
>+  [% IF quip %]
>     [% DEFAULT quip = "$terms.Bugzilla would like to put a random quip here, but no one has entered any." %]

If the DB has no quips, then the default quip will no longer be displayed, even if the user wants them.

Comment 12

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v3

with review comments.

Comment 13

[Simon Green]

Even though it is a performance fix, it's not significant enough that it should be backported to Bugzilla 4.4

Comment 14

[Simon Green]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified buglist.cgi
Committed revision 8754.