----------------------------------------------------------- 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
----------------------------------------------------------- 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
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#file232line293
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#file240line190
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
----------------------------------------------------------- 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
----------------------------------------------------------- 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
----------------------------------------------------------- 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
qa-devel@lists.fedoraproject.org
-
Martin Krizek