Closed Bug 852750 Opened 7 years ago Closed 7 years ago

Flags requested of me, shows only requested flags in non-restricted bugs


( :: MyDashboard, defect)

Not set





(Reporter: gkw, Assigned: dkl)




(1 file, 2 obsolete files)

The flags requested of me section (e.g. feedback?) seems to show only bugs from non-restricted bugs, not from restricted bugs (e.g. s-s bugs). I had 2 feedback? flags set for me from 2 bugs, one s-s and the other not, but I only saw the one from the non-s-s bug in the Dashboard. I am logged in.

The s-s bug is bug 849014 and the non-s-s bug is bug 850070.

(It will be nice to have confirmation of this, since I'm not too sure myself)
I was not able to explicitly reproduce this issue but it made me think that the way I was querying the database to get the flags using direct SQL is more error prone than should be. This patch simplifies the way I get the flags that are displayed by utilizing the built in functions more for filtering privacy, etc. I want glob to take a look to sanity check before we make this type of change live.

Assignee: nobody → dkl
Ever confirmed: true
Attachment #728971 - Flags: review?(glob)
Comment on attachment 728971 [details] [diff] [review]
Patch to simplify how flags are queried for my dashboard (v1)

Review of attachment 728971 [details] [diff] [review]:

very nice refactoring :) works and for me shows a flag which the current query incorrectly misses.

however this creates a bug object individually for each bug, so the db load is much higher than grabbing all the data in a single query.

as discussed on irc, it should be possible to minimise this by using flag matching, then using a single query to grab the bug's status and summary.

::: extensions/MyDashboard/lib/
@@ +183,5 @@
> +    my $matched = Bugzilla::Flag->match($match_params);
> +
> +    my @flags;
> +    foreach my $flag (@$matched) {
> +        next if (!$flag->bug->isopened && !$include_resolved);

the checkbox is "resolved", however you're filtering on all closed states, not just resolved.  the checkbox's label should probably be changed to "show closed".
Attachment #728971 - Flags: review?(glob) → review-
Not sure if this is much simpler as far as lines of code as the original but here is a revised version that does not load full bug objects. Also I changed "Show Resolved" to "Show Closed" as well.

Please review
Attachment #728971 - Attachment is obsolete: true
Attachment #729760 - Flags: review?(glob)
Duplicate of this bug: 854428
New patch changed to work with commit for bug 856110.

Attachment #729760 - Attachment is obsolete: true
Attachment #729760 - Flags: review?(glob)
Attachment #731281 - Flags: review?(glob)
Comment on attachment 731281 [details] [diff] [review]
Patch to simplify how flags are queried for my dashboard (v3)

Review of attachment 731281 [details] [diff] [review]:


this patch contains trailing whitespace, please remove.

::: extensions/MyDashboard/lib/
@@ +229,5 @@
> +    # or if the user did not want to see closed bugs
> +    my @filtered_flags;
> +    foreach my $flag (@unfiltered_flags) {
> +        # Skip this flag if the bug is not visible to the user
> +        next if !$visible_bugs{$flag->{'bug_id'}};

no need to do the visibility check again
Attachment #731281 - Flags: review?(glob) → review+
Committing to: bzr+ssh://             
modified Bugzilla/
modified extensions/MyDashboard/lib/
modified extensions/MyDashboard/lib/
modified extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl                    
modified extensions/MyDashboard/web/js/flags.js
Committed revision 8717.
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.