135836 - change requests should include expiration details

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P2)

Description

[John Vandenberg]

The email change request send emails to both the new and old email address,
providing directions, however it does not include a date after which those
directions will no longer work.

Comment 1

[Gervase Markham [:gerv]]

This should be a simple text change (3 days, I think it is) and it's a usability
improvement. Moving to 2.16 for consideration.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's a patch - but zeroJ will need to confirm that the text I added actually
matches reality.

Gerv

Comment 3

[John Vandenberg]

The period is three days (see Token::CleanTokenTable).  I think it would be more
appropriate to contain the actual date/time, or direct the user to userprefs
which does display these details.
Also note that CleanTokenTable expires all tokens, including password requests.

Comment 4

[Gervase Markham [:gerv]]

If you want to do the patch, go for it - I'm away this weekend :-)

Gerv

Comment 5

[John Vandenberg]

I have already in bug 134805 attachment v1 :)  Tomorrow I will look into
extracting this part of the patch out now it has been targetted for 2.16.

Comment 6

[Matthew Tuck [:CodeMachine]]

I think the template should take the number from the CGI, take from some
constant variable.

Comment 7

[John Vandenberg]

Attachment: Patch v.2

Comment 8

[Gervase Markham [:gerv]]

I look at the patch and I see Param('maxtokenage'), the first word that springs
to mind is "over-engineered" :-)

Do we really need this to be configurable? I can see why we might want to have
the magic number 3 at only one place in the code, so the one admin who decides
this is wrong can change it, but I don't think it's worthy of a param.

Gerv

Comment 9

[John Vandenberg]

Once the email change process has been re-written, this param will then become
the period a user must wait for their new email address to be activated if they
do not have access the old address.  This param will have more meaning as the
use of Token increases, or it will be split into several significant params.
Mark needs-work and I will strip it out.

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 81469 [details] [diff] [review]
Patch v.2

My point is that we should really define the timeout(s) as a member 
variable in Token.pm; they can be edited there if necessary, but 
they are not going to be changed often enough to make it worth 
adding UI for it. UI's not free.

What else are we planning to use tokens for apart from email 
address change requests?

Gerv

Comment 11

[Matthew Tuck [:CodeMachine]]

I'm swaying on this to be honest.  I think we could just remove the parameter
for the moment and find out if any admins care enough to change the code.

Comment 12

[John Vandenberg]

Attachment: Patch v.3

I doubt that administrators will be happy with the defined period once the
email change process is altered, but that does not need to be addressed atm. 
This patch uses a local variable within Token to define the maximum token age.

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 81478 [details] [diff] [review]
Patch v.3

>+    $vars->{'maxtokenage'} = $maxtokenage;
>+    my $expiration_ts = $token_ts + ($maxtokenage * 86400);
>+    $vars->{'expiration_date'} = time2str("%Y-%m-%d %H:%M", $expiration_ts);

I appreciate we need to use a %Y-%m-%d-style date for the DB, but can 
Date::Format produce something nicer (like "11.34pm on 3rd May, 2002")
for the emails?

>+    $vars->{'maxtokenage'} = $maxtokenage;
>+    my $expiration_ts = $token_ts + ($maxtokenage * 86400);
>+    $vars->{'expiration_date'} = time2str("%Y-%m-%d %H:%M", $expiration_ts);
>+

Here, too.

We may have to format it, and pass it into the template, in two bits (if,
say, we want an intervening word "on".) That's fine.

Actually, for bonus points, pass the timestamp and a reference to time2str 
into the template - then localisers can call time2str there, and format 
the timestamp in the appropriate format for their locale.

[% time2str("%H:%M on %d %m, %Y", expiry_timestamp) %] 
or similar.

One more nit - underscore-separate words in template toolkit variables - 
max_token_age instead of maxtokenage.

Gerv

Comment 14

[John Vandenberg]

Attachment: patch v.4

I have moved the computation of the expiration timestamp into the template to
allow localisers to phrase the token age relative to the issue date.

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 81968 [details] [diff] [review]
patch v.4

r=gerv. Perfect :-)

Gerv

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 81968 [details] [diff] [review]
patch v.4

r= justdave

Comment 17

[Gervase Markham [:gerv]]

Fixed. Thanks, John :-)

Checking in template/en/default/account/email/change-new.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/email/change-new.txt.tmpl,v
 <--  change-new.txt.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/account/email/change-old.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/email/change-old.txt.tmpl,v
 <--  change-old.txt.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/account/password/forgotten-password.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/password/forgotten-password.txt.tmpl,v
 <--  forgotten-password.txt.tmpl
new revision: 1.2; previous revision: 1.1
done

Gerv