282488 - Add date search criteria to profiles activity searches

Status: ASSIGNED

(Bugzilla :: Administration, task)

Description

[GavinS]

As Myk said in bug#225162
Myk> I'd love to see this support a date range criterion in addition to a specific
Myk> user criterion so that we could list all changes to any users within two
Myk> dates.


Myk> (It would then be possible to create an RSS version of the page that users
Myk> could subscribe to in order to receive notice of all changes to user
Myk> accounts.)

Comment 1

[GavinS]

Attachment: Add date search for profiles activity

1) Adds option to search for profile activity from a specified date

2) Adds option to search for profile activity up to a specified date

3) Allows searching across all users, not just 1 specific one

4) When not searching for a specific user, adds an extra column to the activity table showing the changee

It does not do (maybe for future bugs)

a) allow a search over multiple specified users

b) allow searching for profile activity over 1 or more groups

Comment 2

[Frédéric Buclin]

Comment on attachment 292815 [details] [diff] [review]
Add date search for profiles activity

>Index: editusers.cgi

>+    my @wheres = ( ' 1 = 1 ' );

Nit: maybe @where would be a better name.


>+        push ( @wheres,
>+               'profiles_activity.userid = ?' );

Nit: this line and the following one could be on a single line.

More critical: I see no visibility restriction if usevisibilitygroup is enabled and you don't have editusers privs (see the 'list' block).


>+        if ( validate_date ( $from_date ) ) {

Nit: don't add a whitespace after the function name.


>+            # Safe, because we have validated, and will use in a
>+            # SELECT via a placehoolder

Nit: placeholder


>+                p2.login_name AS changee

Nit: s/p2/changee/ maybe?



>Index: template/en/default/account/profile-activity.html.tmpl

>+  #   changee: string; loginname of the changed account

Nit: login name (with a whitespace).


>+  [% IF from%]

Missing whitespace -> [% IF from %]


>+   [% columns.push({ name => 'changee',
>+                     heading => 'Changed Account Of' } ); %]

Maybe 'Edited Account' or 'Account' alone?


All this looks great but where is the UI to enter a start and end dates? Maybe you forgot to attach another template to your patch.

Comment 3

[GavinS]

Attachment: Add date based search to profile activity

Now with added UI!

Comment 4

[Frédéric Buclin]

Comment on attachment 311666 [details] [diff] [review]
Add date based search to profile activity

>Index: Bugzilla/DB.pm

>+sub sqlify_date {

This method has nothing to do with databases. It should be in Bugzilla/Util.pm, not here.

Comment 5

[GavinS]

(In reply to comment #4)
> (From update of attachment 311666 [details] [diff] [review])
> >Index: Bugzilla/DB.pm
> 
> >+sub sqlify_date {
> 
> This method has nothing to do with databases. It should be in Bugzilla/Util.pm,
> not here.

Yes, I originally moved it there, but there is a circular dependency because sqlify_date() wants to throw errors, so it needs Error.pm, which itself uses Util.pm.

I'll happily move sqlify_date() elsewhere, if you have another suggestion (or a solution to the circular dependency problem)

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> Yes, I originally moved it there, but there is a circular dependency because
> sqlify_date() wants to throw errors, so it needs Error.pm, which itself uses
> Util.pm.

This means you cannot |use Bugzilla::Error| from Bugzilla::Util, but you can still |require Bugzilla::Error| from sqlify_date(). Add this right before calling Bugzilla::Error::ThrowUserError() and add a comment why we have to require it instead of use it.

Comment 7

[GavinS]

(In reply to comment #6)
> This means you cannot |use Bugzilla::Error| from Bugzilla::Util, but you can
> still |require Bugzilla::Error| from sqlify_date(). Add this right before
> calling Bugzilla::Error::ThrowUserError() and add a comment why we have to
> require it instead of use it.

bug#360626 covers a similar problem. bug#360626 comment#1 says that throwing errors were removed from Util.pm - so should we really be adding them back?

Comment 8

[GavinS]

Attachment: Add date based search to profile activity v3

Here is the version with sqlify_date moved back to Util.pm, if that is preferred

Comment 9

[Teemu Mannermaa (:wicked)]

Comment on attachment 311666 [details] [diff] [review]
Add date based search to profile activity

Yep, sqlify_date sub should really be placed elsewhere.

Comment 10

[Frédéric Buclin]

Comment on attachment 321529 [details] [diff] [review]
Add date based search to profile activity v3

>Index: Bugzilla/Search.pm

>+use Bugzilla::DB;

No reason to add this line, from what I can see.


>+            $dbh->quote(Bugzilla::Util::sqlify_date($chfieldfrom)):'';

Export sqlify_date() in Bugzilla::Util.


When both comments are fixed, ask pyrzak for a first review.

Comment 11

[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.