Add refresh support to the console page.

Add refresh support to the console page.

Use a template for console data:
  - Embeds the refresh value based on a max of the
    selected refresh or 30 seconds.
  - Allows showing the user sign in/sign out links.
  - Move the console header div into the template.
  - Rename console.html to merger.html.
  - Remove grid and t-grid links.
  - Modify get_and_cache_page to return a dict
    that contains content and other data, not just
    the content itself.  Modify that method's
    callers to support the new return data.
  - Update the rest of the page caching to support
    storing and retrieving a serialized dict.

Sign in support:
  - Add utils module to allow us to enforce signed
    in/admin only pages.
  - Move request handler into base_page.
  - Add login/admin helper methods to BasePage.
  - oauth support is absent.

Add tests for the fetch pipeline.
  - Add FakeResponse object to keep fetch_page()
    happy.

Miscellanous fixes:
  - Fix path for gae-lib-root in docs.
  - Remove unused methods (like _clean_int()) and
    imports.
  - Remove a TODO I fixed in a previous CL.
  - Misc formatting fixes.
  - Add W0232 pylint error to suppression.
  - Ignore pylint errors in webapp2.RequestHandler.

R=maruel@chromium.org
BUG=113488
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=139357

[cmp, 2012-05-29 18:35:13]

It's not explicit here, so let me make it clear that my intention is for the app
to support refresh only for signed in users.  I plan to expand on this in a
future change to allow the app to know who is 'signed in' here at some point and
to provide more data that is user-specific (like row highlighting which is a
carry over from the chrome extension and other data), while de-incentivizing
anonymous usage where the user has a choice for the former.

[M-A Ruel, 2012-05-29 18:46:33]

https://chromiumcodereview.appspot.com/10448057/diff/6003/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/PRESUBMIT.py#newcode76
PRESUBMIT.py:76: 'E1101',  # has no member
If possible, you should try fixing it, it's a good warning.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py
File app.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode58
app.py:58: return True
why return something at all?

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode61
app.py:61: # Assumes localpath is already unquoted.
This should go in a docstring to explain what the function does and why it's
looking up content_blob?

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode308
app.py:308: def console_handler(unquoted_localpath, remoteurl, page_data=None):
Use a leading underscore to silence the warning, e.g. _unquoted_localpath.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode415
app.py:415: if md:
Are these expected to fail in the normal case? Because otherwise it'd probably
be better to fail explicitly than failing silently.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode765
app.py:765: page_data = {
you could make a page.as_dict() member function, then you wouldn't need the 4
new members.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app_test.py
File app_test.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/app_test.py#newcode366
app_test.py:366: with open(os.path.join(test_dir, 'expected.html')) as fh:
since you are doing this a few times, it'd be better to use an utility member
function so you can do:

expected_content = self._load_content('expected.html')

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py
File base_page.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode12
base_page.py:12: # F0401:  9,0: Unable to import 'webapp2'
You should add it to sys.path in the PRESUBMIT check?

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode18
base_page.py:18: # W0232: 23,0:MyRequestHandler: Class has no __init__ method
I thought I had disabled this warning, feel free to disable it everywhere.

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode23
base_page.py:23: _VALID_EMAIL = re.compile(r"^.*@(chromium\.org|google\.com)$")
replace .* with .+ :)

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode57
base_page.py:57: if self.user:
optional style nit: I'd make that a one line:
nickname = self.user.email() if self.user else ''

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode76
base_page.py:76: Optionally cache it for 1 second. Only to be used for
user-invariant
Any way to assert this programmatically? I don't think it's possible, just
asking in case you have an idea.

https://chromiumcodereview.appspot.com/10448057/diff/6003/handler.py
File handler.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/handler.py#newcode41
handler.py:41: template_values['body_class'] = page_data.get('body_class')
optional: would also be a perfect fit for page_data.as_dict() as in:

template_values.update(page_data.as_dict())

https://chromiumcodereview.appspot.com/10448057/diff/6003/tests/test_fetch_co...
File tests/test_fetch_console/expected.html (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/tests/test_fetch_co...
tests/test_fetch_console/expected.html:182: <script>None</script>,<br>
That's expected by the unit test?

[cmp, 2012-05-29 19:38:03]

patch updated, PTAL

https://chromiumcodereview.appspot.com/10448057/diff/6003/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/PRESUBMIT.py#newcode76
PRESUBMIT.py:76: 'E1101',  # has no member
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> If possible, you should try fixing it, it's a good warning.

I disabled this because it gives false positives due to webapp2 and AppEngine's
SDK:

************* Module app_test
E1101: 43,16:GaeTestCase.clear_datastore: Instance of 'APIProxyStubMap' has no
'_APIProxyStubMap__stub_map' member
E1101: 47,12:GaeTestCase.clear_datastore: Instance of 'APIProxyStubMap' has no
'_APIProxyStubMap__stub_map' member
************* Module base_page
E1101: 65,42:BasePage.InitializeTemplate: Instance of 'BasePage' has no
'request' member
E1101: 66,44:BasePage.InitializeTemplate: Instance of 'BasePage' has no
'request' member
E1101: 79,4:BasePage.DisplayTemplate: Instance of 'BasePage' has no 'response'
member
E1101: 88,4:BasePage.DisplayTemplate: Instance of 'BasePage' has no 'response'
member
************* Module handler
E1101: 24,11:PageAction.get: Instance of 'PageAction' has no 'request' member
E1101: 26,13:PageAction.get: Instance of 'PageAction' has no 'request' member
E1101: 33,6:PageAction.get: Instance of 'PageAction' has no 'error' member
E1101: 36,4:PageAction.get: Instance of 'PageAction' has no 'response' member
E1101: 39,7:PageAction.get: Instance of 'PageAction' has no 'request' member
E1101: 46,34:PageAction.get: Instance of 'PageAction' has no 'request' member
E1101: 54,8:PageAction.get: Instance of 'PageAction' has no 'response' member
E1101: 55,8:PageAction.get: Instance of 'PageAction' has no 'response' member
E1101: 61,4:PageAction.get: Instance of 'PageAction' has no 'response' member
E1101: 62,4:PageAction.get: Instance of 'PageAction' has no 'response' member
E1101: 63,4:PageAction.get: Instance of 'PageAction' has no 'response' member
E1101: 78,4:MainAction.get: Instance of 'MainAction' has no 'redirect' member

You suggested elsewhere not explicitly marking each site but to use a general
exemption, so I used this instead.  I would like to keep it, let me know if you
have suggestions for an alternative approach.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py
File app.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode58
app.py:58: return True
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> why return something at all?

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode61
app.py:61: # Assumes localpath is already unquoted.
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> This should go in a docstring to explain what the function does and why it's
> looking up content_blob?

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode308
app.py:308: def console_handler(unquoted_localpath, remoteurl, page_data=None):
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> Use a leading underscore to silence the warning, e.g. _unquoted_localpath.

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode415
app.py:415: if md:
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> Are these expected to fail in the normal case? Because otherwise it'd probably
> be better to fail explicitly than failing silently.

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/app.py#newcode765
app.py:765: page_data = {
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> you could make a page.as_dict() member function, then you wouldn't need the 4
> new members.

page_data is not really a Page-as-dict.  It's more like a
what-does-the-console-template-need-from-this-Page-object.  But I see your
point, and maybe a method to get that would help.  In other places, I create
such a dict without having a Page object handy (like in fetch_page or in one of
the caching methods).  If I moved these there, I think I'd still end up having
some kind of abstraction leak due to these other places.  Do you have more ideas
about that?

https://chromiumcodereview.appspot.com/10448057/diff/6003/app_test.py
File app_test.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/app_test.py#newcode366
app_test.py:366: with open(os.path.join(test_dir, 'expected.html')) as fh:
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> since you are doing this a few times, it'd be better to use an utility member
> function so you can do:
> 
> expected_content = self._load_content('expected.html')

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py
File base_page.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode12
base_page.py:12: # F0401:  9,0: Unable to import 'webapp2'
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> You should add it to sys.path in the PRESUBMIT check?

This is provided by GAE SDK.  It is not a submodule of the google module,
though.  It is in lib/, and that ends up in PYTHONPATH due to appcfg's
fix_sys_path().  Since the path to appengine can be different per local dev
checkout, it seems like the right thing to do is to run fix_sys_path where the
presubmit tests locate the GAE SDK.

I made that change and removed this pylint disable line.

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode18
base_page.py:18: # W0232: 23,0:MyRequestHandler: Class has no __init__ method
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> I thought I had disabled this warning, feel free to disable it everywhere.

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode23
base_page.py:23: _VALID_EMAIL = re.compile(r"^.*@(chromium\.org|google\.com)$")
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> replace .* with .+ :)

Done.  You should look at chromium-status, the same thing is there (unless I was
in an old checkout).

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode57
base_page.py:57: if self.user:
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> optional style nit: I'd make that a one line:
> nickname = self.user.email() if self.user else ''

Done.

https://chromiumcodereview.appspot.com/10448057/diff/6003/base_page.py#newcode76
base_page.py:76: Optionally cache it for 1 second. Only to be used for
user-invariant
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> Any way to assert this programmatically? I don't think it's possible, just
> asking in case you have an idea.

Nothing quick and easy that I can think of.  Mostly solutions that hash content
per URL and compare the hashes across users come to mind.  The problem I can't
quickly solve is that the content can change based on a number of other factors
(so the problem is more accurately phrased: only to be used for pages that are
variant and not user-variant, or for invariant pages).

I'm not too concerned about that right now since I don't set use_cache anywhere
and have business logic rules around how things should be cached and written to
output.  I understand why you're asking though.  LMK if you think of anything.

https://chromiumcodereview.appspot.com/10448057/diff/6003/handler.py
File handler.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/handler.py#newcode41
handler.py:41: template_values['body_class'] = page_data.get('body_class')
On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> optional: would also be a perfect fit for page_data.as_dict() as in:
> 
> template_values.update(page_data.as_dict())

Yeah, I agree.  If you have an idea in the other comment about how to handle the
non-from-Page cases and I end up using something like this, I'll come back to
this, too.

https://chromiumcodereview.appspot.com/10448057/diff/6003/tests/test_fetch_co...
File tests/test_fetch_console/expected.html (right):

https://chromiumcodereview.appspot.com/10448057/diff/6003/tests/test_fetch_co...
tests/test_fetch_console/expected.html:182: <script>None</script>,<br>
Yes, that's expected.

I don't prime the app first with other sheriff values, so that's why None show's
up here.  This test is meant to show that fetch_page with the console_handler
hooked up does the right thing, which verifies that page_data is passed around
correctly and returned properly.  That None is written shows that document.write
is being correctly substituted, along with the other changes that
console_handler is making to this output.

FYI this is a new file that didn't exist before.  You see a previous copy on the
left because I have 'git diff -C -C' in my depot_tools.

[M-A Ruel, 2012-05-29 20:03:49]

lgtm

https://chromiumcodereview.appspot.com/10448057/diff/13014/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/13014/PRESUBMIT.py#newco...
PRESUBMIT.py:79: 'E1101',  # has no member, disabled due to webapp2 and
AppEngine SDK
An option would be to add 'webapp2.RequestHandler' to ignored-classes= line in
pylintrc. I haven't exposed a way to set this dynamically in the presubmit
canned check though.

https://chromiumcodereview.appspot.com/10448057/diff/13014/app.py
File app.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/13014/app.py#newcode61
app.py:61: """Return a page_data dict, optionally caching and looking up a blob.
Returns

https://chromiumcodereview.appspot.com/10448057/diff/13014/app.py#newcode782
app.py:782: page_data = {
On 2012/05/29 19:38:03, cmp wrote:
> On 2012/05/29 18:46:33, Marc-Antoine Ruel wrote:
> > you could make a page.as_dict() member function, then you wouldn't need the
4
> > new members.
> 
> page_data is not really a Page-as-dict.  It's more like a
> what-does-the-console-template-need-from-this-Page-object.  But I see your
> point, and maybe a method to get that would help.  In other places, I create
> such a dict without having a Page object handy (like in fetch_page or in one
of
> the caching methods).  If I moved these there, I think I'd still end up having
> some kind of abstraction leak due to these other places.  Do you have more
ideas
> about that?

I agree the function shouldn't be named as_dict() but something more specific. I
also agree it's not a perfect mapping but it's also overkill to define an
embedded class, and it's not really easy to do in python for serialized data.

It's used 3 times so it's still maybe worth doing one;
app.py:88
app.py:782
handler.py:39

I don't mind, it's up to you.

[cmp, 2012-05-29 20:20:28]

https://chromiumcodereview.appspot.com/10448057/diff/13014/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/13014/PRESUBMIT.py#newco...
PRESUBMIT.py:79: 'E1101',  # has no member, disabled due to webapp2 and
AppEngine SDK
On 2012/05/29 20:03:49, Marc-Antoine Ruel wrote:
> An option would be to add 'webapp2.RequestHandler' to ignored-classes= line in
> pylintrc. I haven't exposed a way to set this dynamically in the presubmit
> canned check though.

Done.

https://chromiumcodereview.appspot.com/10448057/diff/13014/app.py
File app.py (right):

https://chromiumcodereview.appspot.com/10448057/diff/13014/app.py#newcode61
app.py:61: """Return a page_data dict, optionally caching and looking up a blob.
On 2012/05/29 20:03:49, Marc-Antoine Ruel wrote:
> Returns

Done.

https://chromiumcodereview.appspot.com/10448057/diff/13014/app.py#newcode782
app.py:782: page_data = {
On 2012/05/29 20:03:49, Marc-Antoine Ruel wrote:
> I agree the function shouldn't be named as_dict() but something more specific.
I
> also agree it's not a perfect mapping but it's also overkill to define an
> embedded class, and it's not really easy to do in python for serialized data.
> 
> It's used 3 times so it's still maybe worth doing one;
> app.py:88
> app.py:782
> handler.py:39
> 
> I don't mind, it's up to you.

My preference given we're both on the fence is to leave it as-is for now and
revisit in a future CL as needed then.