637007 - undefined name 'msg' in console

Status: RESOLVED FIXED

(Cloud Services :: Server: Account Portal, defect)

Description

[Tarek Ziadé (:tarek)]

Attachment: Flake8 pass

Prior to fix a test (http://hg.mozilla.org/services/account-portal/rev/ac6075a51ba1) I've run Flake8 on the code base.

Find attached all the cleanup fixes for review.

There's one remaining problem that needs a test+fix in console.py in the forgot() method that will break the app.

Comment 1

[Toby Elliott [:telliott]]

There seem to be a ton of changes here, not all of which make sense to me, and I'm not sure what the actual bug is.

Comment 2

[Toby Elliott [:telliott]]

Comment on attachment 515340 [details] [diff] [review]
Flake8 pass

Not thrilled by the ones that are removing the dropped-}, and I'm confused by the one that imports _, since that should be broken in about 50 places. The rest seem fine.

Comment 3

[Tarek Ziadé (:tarek)]

(In reply to comment #2)
> Comment on attachment 515340 [details] [diff] [review]
> Flake8 pass
> 
> Not thrilled by the ones that are removing the dropped-},

yeah -- that could be changed in flake8

> and I'm confused by
> the one that imports _, since that should be broken in about 50 places. The
> rest seem fine.

The magic behind gettext.install() that injects _ in the builtins, is not see by the Flake8 script which inspect the AST.  This import is not hurting and fixes the warnings.  We could improve this by ignoring warnings related to _ undefined names.

Pushed in http://hg.mozilla.org/services/account-portal/rev/880244b4189f

The last remaining issue is now the undefined name.

Comment 4

[Toby Elliott [:telliott]]

(In reply to comment #3)

> The magic behind gettext.install() that injects _ in the builtins, is not see
> by the Flake8 script which inspect the AST.  This import is not hurting and
> fixes the warnings.  We could improve this by ignoring warnings related to _
> undefined names.
> 

Right. My question is - if it's complaining about this in console.py, why isn't it also complaining about it in sync.py?

Comment 5

[Toby Elliott [:telliott]]

Attachment: removes the old message

I see the issue. We had a stray error left over from when the ap was sending the email. It just needs pulling.

I would expect the backend to be logging details of a failure, so we just need something informational here to alert folks that there might be a problem. This level of code has no idea why.

Comment 6

[Tarek Ziadé (:tarek)]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > The magic behind gettext.install() that injects _ in the builtins, is not see
> > by the Flake8 script which inspect the AST.  This import is not hurting and
> > fixes the warnings.  We could improve this by ignoring warnings related to _
> > undefined names.
> > 
> 
> Right. My question is - if it's complaining about this in console.py, why isn't
> it also complaining about it in sync.py?

If you don't see them, that might be related to the limited size of warnings in flake8. The stack of warnings for each kind of warnings is limited. 

If you fix just console.py, you should see them in sync.py then. That's yet another thing I need to fix in flake8

Comment 7

[Toby Elliott [:telliott]]

Ah, ok. You'd sort of implied that this was the definitive list of fixes.

That would be a tremendously useful enhancement in flake8. It's really annoying to make all the fixes in a file, then discover another one was being masked because the same class of error was in another file.

Comment 8

[Tarek Ziadé (:tarek)]

Comment on attachment 516056 [details] [diff] [review]
removes the old message

Since this was not making any test fail, we need to add with the fix a test that exercise this password_reset1 screen.

This can be done by patching temporarily generate_reset_code in the test so it returns False

Comment 9

[Tarek Ziadé (:tarek)]

what's the status on this one ?

Comment 10

[Toby Elliott [:telliott]]

I think the last actual bug part of this got worked around ages ago.