Review Request 17: Implement Bugzilla Account Association

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/17/
-----------------------------------------------------------

Review request for blockerbugs.


Bugs: 347
    https://fedorahosted.org/fedora-qa/ticket/347


Repository: blockerbugs


Description
-------

This is a patch for tickets 347, 348, 349 and 350.


Diffs
-----

  testing/test_bugchange.py 65f291122b4ae687872170517d196a621d548152 
  blockerbugs/util/bz_interface.py a8958be674e839d6c99694e33440a26331a8b041 
  blockerbugs/templates/propose_bug.html c97d72303dff2f49e4bb8bc5ff5fc2f4c764ce8b 
  blockerbugs/templates/login.html 20674fe93eb96c6e1e9b12b5988cad67dc1a5181 
  blockerbugs/templates/layout.html 6d0ae9a7bc913aa408985028db4494d51379da9c 
  blockerbugs/templates/fas_bugzilla.html PRE-CREATION 
  blockerbugs/templates/base_nav.html ce7a7bb478ad57a65d646fa7326a210c6a4eb131 
  blockerbugs/models/userinfo.py PRE-CREATION 
  blockerbugs/controllers/users.py 30b5fb5c8ef2196979a2d22c4380f302a5ce951a 
  blockerbugs/controllers/main.py ce26c641b9db05f11510f16177b017a56cf757c2 
  blockerbugs/controllers/forms.py b213c055d9297b59c6cf5d83358dcd14e265e345 
  alembic/versions/1d12b74d12bd_add_userinfo_table.py PRE-CREATION 

Diff: http://reviewboard-tflink.rhcloud.com/r/17/diff/


Testing
-------


Thanks,

Martin Krizek

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/17/#review17
-----------------------------------------------------------


I had a couple of questions and some minor concerns but it looks pretty good overall.


blockerbugs/controllers/main.py
<http://reviewboard-tflink.rhcloud.com/r/17/#comment22>

    How come you broke this up as a separate function? It seems a bit stylistically
different from the rest of the code



blockerbugs/models/userinfo.py
<http://reviewboard-tflink.rhcloud.com/r/17/#comment25>

    do we know if there are limits on the possible length of these usernames? I assume
that there are but then again, I made the same assumption about other fields as well



blockerbugs/templates/fas_bugzilla.html
<http://reviewboard-tflink.rhcloud.com/r/17/#comment23>

    Stylistically, this doesn't match the existing login page that we have for FAS -
it would be better if they matched



blockerbugs/util/bz_interface.py
<http://reviewboard-tflink.rhcloud.com/r/17/#comment24>

    If we get here, the app will blow up with an unhandled exception which originates from
my code but as I'm looking at it again, I'm wondering about having a try/catch in
the controller which sets an error in the proposal form and lets the user re-submit


- Tim Flink


On March 13, 2013, 10:32 a.m., Martin Krizek wrote:
>  
>  -----------------------------------------------------------
>  This is an automatically generated e-mail. To reply, visit:
>  http://reviewboard-tflink.rhcloud.com/r/17/
>  -----------------------------------------------------------
>  
>  (Updated March 13, 2013, 10:32 a.m.)
>  
>  
>  Review request for blockerbugs.
>  
>  
>  Bugs: 347
>      https://fedorahosted.org/fedora-qa/ticket/347
>  
>  
>  Repository: blockerbugs
>  
>  
>  Description
>  -------
>  
>  This is a patch for tickets 347, 348, 349 and 350.
>  
>  
>  Diffs
>  -----
>  
>    testing/test_bugchange.py 65f291122b4ae687872170517d196a621d548152 
>    blockerbugs/util/bz_interface.py a8958be674e839d6c99694e33440a26331a8b041 
>    blockerbugs/templates/propose_bug.html c97d72303dff2f49e4bb8bc5ff5fc2f4c764ce8b 
>    blockerbugs/templates/login.html 20674fe93eb96c6e1e9b12b5988cad67dc1a5181 
>    blockerbugs/templates/layout.html 6d0ae9a7bc913aa408985028db4494d51379da9c 
>    blockerbugs/templates/fas_bugzilla.html PRE-CREATION 
>    blockerbugs/templates/base_nav.html ce7a7bb478ad57a65d646fa7326a210c6a4eb131 
>    blockerbugs/models/userinfo.py PRE-CREATION 
>    blockerbugs/controllers/users.py 30b5fb5c8ef2196979a2d22c4380f302a5ce951a 
>    blockerbugs/controllers/main.py ce26c641b9db05f11510f16177b017a56cf757c2 
>    blockerbugs/controllers/forms.py b213c055d9297b59c6cf5d83358dcd14e265e345 
>    alembic/versions/1d12b74d12bd_add_userinfo_table.py PRE-CREATION 
>  
>  Diff: http://reviewboard-tflink.rhcloud.com/r/17/diff/
>  
>  
>  Testing
>  -------
>  
>  
>  Thanks,
>  
>  Martin Krizek
>  
>

[Martin Krizek]

>  On March 13, 2013, 9:32 p.m., Tim Flink wrote:
>  > blockerbugs/controllers/main.py, line 293
>  > <http://reviewboard-tflink.rhcloud.com/r/17/diff/1/?file=232#file232line29...
>  >
>  >     How come you broke this up as a separate function? It seems a bit stylistically
> different from the rest of the code 
Initially, there were some checks that were not necessary in the end. I moved the code out
of the function. Good point.


>  On March 13, 2013, 9:32 p.m., Tim Flink wrote:
>  > blockerbugs/models/userinfo.py, line 28
>  > <http://reviewboard-tflink.rhcloud.com/r/17/diff/1/?file=234#file234line28...
>  >
>  >     do we know if there are limits on the possible length of these usernames? I
> assume that there are but then again, I made the same assumption about other fields as
> well 
First I thought that usernames are of an e-mail form (which is not true) so that's why
I put '255'. However I think it's safe to assume no one will have username
longer than 255 characters, right?


>  On March 13, 2013, 9:32 p.m., Tim Flink wrote:
>  > blockerbugs/templates/fas_bugzilla.html, line 30
>  > <http://reviewboard-tflink.rhcloud.com/r/17/diff/1/?file=236#file236line30...
>  >
>  >     Stylistically, this doesn't match the existing login page that we have for
> FAS - it would be better if they matched 
Fair enough, fixed.


>  On March 13, 2013, 9:32 p.m., Tim Flink wrote:
>  > blockerbugs/util/bz_interface.py, line 190
>  > <http://reviewboard-tflink.rhcloud.com/r/17/diff/1/?file=240#file240line19...
>  >
>  >     If we get here, the app will blow up with an unhandled exception which
> originates from my code but as I'm looking at it again, I'm wondering about having
> a try/catch in the controller which sets an error in the proposal form and lets the user
> re-submit 
So I put try/catch block in the controller, let me know what you think.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/17/#review17
-----------------------------------------------------------


On March 14, 2013, 11:24 a.m., Martin Krizek wrote:
>  
>  -----------------------------------------------------------
>  This is an automatically generated e-mail. To reply, visit:
>  http://reviewboard-tflink.rhcloud.com/r/17/
>  -----------------------------------------------------------
>  
>  (Updated March 14, 2013, 11:24 a.m.)
>  
>  
>  Review request for blockerbugs.
>  
>  
>  Bugs: 347
>      https://fedorahosted.org/fedora-qa/ticket/347
>  
>  
>  Repository: blockerbugs
>  
>  
>  Description
>  -------
>  
>  This is a patch for tickets 347, 348, 349 and 350.
>  
>  
>  Diffs
>  -----
>  
>    testing/test_bugchange.py 65f291122b4ae687872170517d196a621d548152 
>    blockerbugs/util/bz_interface.py a8958be674e839d6c99694e33440a26331a8b041 
>    blockerbugs/templates/propose_bug.html c97d72303dff2f49e4bb8bc5ff5fc2f4c764ce8b 
>    blockerbugs/templates/login.html 20674fe93eb96c6e1e9b12b5988cad67dc1a5181 
>    blockerbugs/templates/layout.html 6d0ae9a7bc913aa408985028db4494d51379da9c 
>    blockerbugs/templates/fas_bugzilla.html PRE-CREATION 
>    blockerbugs/templates/base_nav.html ce7a7bb478ad57a65d646fa7326a210c6a4eb131 
>    blockerbugs/models/userinfo.py PRE-CREATION 
>    blockerbugs/controllers/users.py 30b5fb5c8ef2196979a2d22c4380f302a5ce951a 
>    blockerbugs/controllers/main.py ce26c641b9db05f11510f16177b017a56cf757c2 
>    blockerbugs/controllers/forms.py b213c055d9297b59c6cf5d83358dcd14e265e345 
>    alembic/versions/1d12b74d12bd_add_userinfo_table.py PRE-CREATION 
>  
>  Diff: http://reviewboard-tflink.rhcloud.com/r/17/diff/
>  
>  
>  Testing
>  -------
>  
>  
>  Thanks,
>  
>  Martin Krizek
>  
>

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/17/
-----------------------------------------------------------

(Updated March 14, 2013, 11:24 a.m.)


Review request for blockerbugs.


Bugs: 347
    https://fedorahosted.org/fedora-qa/ticket/347


Repository: blockerbugs


Description
-------

This is a patch for tickets 347, 348, 349 and 350.


Diffs (updated)
-----

  testing/test_bugchange.py 65f291122b4ae687872170517d196a621d548152 
  blockerbugs/util/bz_interface.py a8958be674e839d6c99694e33440a26331a8b041 
  blockerbugs/templates/propose_bug.html c97d72303dff2f49e4bb8bc5ff5fc2f4c764ce8b 
  blockerbugs/templates/login.html 20674fe93eb96c6e1e9b12b5988cad67dc1a5181 
  blockerbugs/templates/layout.html 6d0ae9a7bc913aa408985028db4494d51379da9c 
  blockerbugs/templates/fas_bugzilla.html PRE-CREATION 
  blockerbugs/templates/base_nav.html ce7a7bb478ad57a65d646fa7326a210c6a4eb131 
  blockerbugs/models/userinfo.py PRE-CREATION 
  blockerbugs/controllers/users.py 30b5fb5c8ef2196979a2d22c4380f302a5ce951a 
  blockerbugs/controllers/main.py ce26c641b9db05f11510f16177b017a56cf757c2 
  blockerbugs/controllers/forms.py b213c055d9297b59c6cf5d83358dcd14e265e345 
  alembic/versions/1d12b74d12bd_add_userinfo_table.py PRE-CREATION 

Diff: http://reviewboard-tflink.rhcloud.com/r/17/diff/


Testing
-------


Thanks,

Martin Krizek

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/17/#review19
-----------------------------------------------------------


Looks great overall. I had a few more comments but they're minor and I don't think
that their fixes will require another review - feel free to merge into develop after
you're done with the comments unless you have concerns about them.


blockerbugs/controllers/users.py
<http://reviewboard-tflink.rhcloud.com/r/17/#comment31>

    I don't think this is enough to stop these changes but we should probably start
using the flash messages in other places than the bz association page.
    
    If you've logged in and out before doing the association, the flash messages pile
up and you end up with multiple login successful messages on the bz account association
page



blockerbugs/templates/fas_bugzilla.html
<http://reviewboard-tflink.rhcloud.com/r/17/#comment32>

    I'm being a little nit-picky here but the wording of this title is a bit akward.
    
    What about "Link your FAS and Red Hat Bugzilla Accounts"?



blockerbugs/templates/fas_bugzilla.html
<http://reviewboard-tflink.rhcloud.com/r/17/#comment30>

    can you either remove the fieldset around this login or add it to the FAS login page
to make them consistent?



blockerbugs/templates/fas_bugzilla.html
<http://reviewboard-tflink.rhcloud.com/r/17/#comment33>

    Can you stick a bit of text at the end of this which says:
    
    "Your Red Hat bugzilla password is used only to verify that you control the
account which will be linked to your FAS account. Your password will
<b>NOT</b> be stored anywhere in this application."


- Tim Flink


On March 14, 2013, 11:24 a.m., Martin Krizek wrote:
>  
>  -----------------------------------------------------------
>  This is an automatically generated e-mail. To reply, visit:
>  http://reviewboard-tflink.rhcloud.com/r/17/
>  -----------------------------------------------------------
>  
>  (Updated March 14, 2013, 11:24 a.m.)
>  
>  
>  Review request for blockerbugs.
>  
>  
>  Bugs: 347
>      https://fedorahosted.org/fedora-qa/ticket/347
>  
>  
>  Repository: blockerbugs
>  
>  
>  Description
>  -------
>  
>  This is a patch for tickets 347, 348, 349 and 350.
>  
>  
>  Diffs
>  -----
>  
>    testing/test_bugchange.py 65f291122b4ae687872170517d196a621d548152 
>    blockerbugs/util/bz_interface.py a8958be674e839d6c99694e33440a26331a8b041 
>    blockerbugs/templates/propose_bug.html c97d72303dff2f49e4bb8bc5ff5fc2f4c764ce8b 
>    blockerbugs/templates/login.html 20674fe93eb96c6e1e9b12b5988cad67dc1a5181 
>    blockerbugs/templates/layout.html 6d0ae9a7bc913aa408985028db4494d51379da9c 
>    blockerbugs/templates/fas_bugzilla.html PRE-CREATION 
>    blockerbugs/templates/base_nav.html ce7a7bb478ad57a65d646fa7326a210c6a4eb131 
>    blockerbugs/models/userinfo.py PRE-CREATION 
>    blockerbugs/controllers/users.py 30b5fb5c8ef2196979a2d22c4380f302a5ce951a 
>    blockerbugs/controllers/main.py ce26c641b9db05f11510f16177b017a56cf757c2 
>    blockerbugs/controllers/forms.py b213c055d9297b59c6cf5d83358dcd14e265e345 
>    alembic/versions/1d12b74d12bd_add_userinfo_table.py PRE-CREATION 
>  
>  Diff: http://reviewboard-tflink.rhcloud.com/r/17/diff/
>  
>  
>  Testing
>  -------
>  
>  
>  Thanks,
>  
>  Martin Krizek
>  
>

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/17/
-----------------------------------------------------------

(Updated March 15, 2013, 9:19 a.m.)


Status
------

This change has been marked as submitted.


Review request for blockerbugs.


Bugs: 347
    https://fedorahosted.org/fedora-qa/ticket/347


Repository: blockerbugs


Description
-------

This is a patch for tickets 347, 348, 349 and 350.


Diffs
-----

  testing/test_bugchange.py 65f291122b4ae687872170517d196a621d548152 
  blockerbugs/util/bz_interface.py a8958be674e839d6c99694e33440a26331a8b041 
  blockerbugs/templates/propose_bug.html c97d72303dff2f49e4bb8bc5ff5fc2f4c764ce8b 
  blockerbugs/templates/login.html 20674fe93eb96c6e1e9b12b5988cad67dc1a5181 
  blockerbugs/templates/layout.html 6d0ae9a7bc913aa408985028db4494d51379da9c 
  blockerbugs/templates/fas_bugzilla.html PRE-CREATION 
  blockerbugs/templates/base_nav.html ce7a7bb478ad57a65d646fa7326a210c6a4eb131 
  blockerbugs/models/userinfo.py PRE-CREATION 
  blockerbugs/controllers/users.py 30b5fb5c8ef2196979a2d22c4380f302a5ce951a 
  blockerbugs/controllers/main.py ce26c641b9db05f11510f16177b017a56cf757c2 
  blockerbugs/controllers/forms.py b213c055d9297b59c6cf5d83358dcd14e265e345 
  alembic/versions/1d12b74d12bd_add_userinfo_table.py PRE-CREATION 

Diff: http://reviewboard-tflink.rhcloud.com/r/17/diff/


Testing
-------


Thanks,

Martin Krizek