672918 - create basic structure/code for balrog's human admin interface

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[[e-mail] (:bhearsum)]

Build machines will need to be able to interact with AUS3 when publishing nightly updates and releases. They need to be able to submit a new release, and possibly make changes to the rules. This could be implemented as a REST API, something else over HTTP, or something else altogether.

If implemented over HTTP, this and bug 672916 might be able to share the backend.

Comment 1

[[e-mail] (:bhearsum)]

I've been working on designing this here: https://wiki.mozilla.org/User:Bhearsum/AUS3/Administration

Comment 2

[[e-mail] (:bhearsum)]

Attachment: structure + /users implementation

This patch is a rough take on the structure of the admin interface for AUS3. I've implemented most of the /users API as a way of getting a feel for things.

Currently, web/base.py has an 'app', which imports all of the existing views. If we built multiple frontends (eg, admin + client) we'd probably need a web/admin.py and web/client.py - which would only import the things they want.

I fiddled with different ways of doing HTML generation for awhile: server side with templates, server side without templates, barebones server side + AJAX loading of content. In the end, I settled on server side with templates because it seemed the simplest -- doing a lot of AJAX loading seemed like I was spreading out the building of the pages unnecessarily.

As noted in the comments, the authentication scheme will need to change when we figure out what we're doing in production. It's isolated in the few methods in web/view/base.py, so it shouldn't be a big deal to change.

This page builds on some more additions to db.py, too. Once I finish addressing the review comments in bug 678163 I'll be posting an updated patch there. If you want to have a look at the current state of things, https://github.com/bhearsum/aus3-proto/blob/history/auslib/db.py has it.

Comment 3

[Chris AtLee [:catlee]]

Comment on attachment 556663 [details] [diff] [review]
structure + /users implementation

Review of attachment 556663 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Let's get rid of the circular imports (base.py -> permissions.py -> base.py); maybe use a setup() function in permissions.py to set up the routes

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 556663 [details] [diff] [review]
structure + /users implementation

Looks good. BTW, we lost auslib/__init__.py somewhere along the way to the hg copy of aus3-proto.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: updated to cope with versioning, a few other small things

Not _too_ much change here. The biggest thing is passing along data_version all over the place to cope with the version checking implemented in bug 678163. The other things in this patch are:
* Small improvements to readability in templates (foo.bar instead of foo['bar'])
* Strip leading slash from permissions to avoid double slashes in form actions (which Flask doesn't deal with very well, apparently)
* Move permission/option parsing into common places

Comment 6

[Chris AtLee [:catlee]]

Comment on attachment 564627 [details] [diff] [review]
updated to cope with versioning, a few other small things

Review of attachment 564627 [details] [diff] [review]:
-----------------------------------------------------------------

::: auslib/web/base.py
@@ +1,3 @@
> +from flask import Flask
> +
> +from sqlalchemy import create_engine

not needed?

Comment 7

[[e-mail] (:bhearsum)]

I just realized that this work was supposed to go in bug 672916 =\. Updating the summary to reflect what's actually gone on here.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 564627 [details] [diff] [review]
updated to cope with versioning, a few other small things

::: auslib/web/base.py
@@ +1,3 @@
> +from flask import Flask
> +
> +from sqlalchemy import create_engine

> not needed?

Landed, and I removed this unneeded import.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: rework html/javascript to make it easier to share across multiple pages; use WTForms

This patch started with just implementing WTForms, but as I started thinking about some of our use cases more it became clear to me that there's going to be cases where we may want to use similar forms or displays of data on multiple different pages. Given that, I decided to factor out things like "create a form for permissions" to a little snippet of jinja2 that can be included onto pages. Those two things are the bulk of this patch. I also:
* Adde some more debugging to AUSTable
* Renamed some tests to something I thought made a bit more sense
* Implemented add/update/delete of permissions, for new and existing users w/ jquery.
* Fix a bug in requirepermission()'s reporting of errors (it used to report all KeyError's as being unable to find "product").

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 605041 [details] [diff] [review]
rework html/javascript to make it easier to share across multiple pages; use WTForms

Review of attachment 605041 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor things I noticed, still need to tackle the html changes.

::: auslib/db.py
@@ +184,5 @@
>             
>             @rtype: sqlalchemy.engine.base.ResultProxy
>          """
>          query = self._selectStatement(**kwargs)
> +        log.debug("AUSTable._prepareUpdate: Executing query: '%s'", query)

nit - this is in select()

@@ +212,5 @@
>          data = columns.copy()
>          if self.versioned:
>              data['data_version'] = 1
> +        query = self._insertStatement(**data)
> +        log.debug("AUSTable._prepareUpdate: Executing query: '%s' with values: %s", query, data)

nit - this is in _prepareInsert()

@@ +274,5 @@
>              where = copy(where)
>              where.append(self.data_version==old_data_version)
>  
> +        query = self._deleteStatement(where)
> +        log.debug("AUSTable._prepareUpdate: Executing query: '%s'", query)

nit - this is in _prepareDelete()

::: auslib/web/templates/base.html
@@ +1,5 @@
>  <!DOCTYPE HTML>
>  <html lang='en-US'>
>  <head>
>  <title>{% block title %}{% endblock %}</title>
> +<link rel='stylesheet' type='text/css' href='{{ url_for('static', filename='ausadmin.css') }}' />

ausadmin.css isn't included in the patch, should it be ?

Comment 12

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #11)
> Comment on attachment 605041 [details] [diff] [review]
> rework html/javascript to make it easier to share across multiple pages; use
> WTForms
> 
> Review of attachment 605041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some minor things I noticed, still need to tackle the html changes.
> 
> ::: auslib/db.py
> @@ +184,5 @@
> >             
> >             @rtype: sqlalchemy.engine.base.ResultProxy
> >          """
> >          query = self._selectStatement(**kwargs)
> > +        log.debug("AUSTable._prepareUpdate: Executing query: '%s'", query)
> 
> nit - this is in select()
> 
> @@ +212,5 @@
> >          data = columns.copy()
> >          if self.versioned:
> >              data['data_version'] = 1
> > +        query = self._insertStatement(**data)
> > +        log.debug("AUSTable._prepareUpdate: Executing query: '%s' with values: %s", query, data)
> 
> nit - this is in _prepareInsert()
> 
> @@ +274,5 @@
> >              where = copy(where)
> >              where.append(self.data_version==old_data_version)
> >  
> > +        query = self._deleteStatement(where)
> > +        log.debug("AUSTable._prepareUpdate: Executing query: '%s'", query)
> 
> nit - this is in _prepareDelete()

Whoops! Will fix these up.

> ::: auslib/web/templates/base.html
> @@ +1,5 @@
> >  <!DOCTYPE HTML>
> >  <html lang='en-US'>
> >  <head>
> >  <title>{% block title %}{% endblock %}</title>
> > +<link rel='stylesheet' type='text/css' href='{{ url_for('static', filename='ausadmin.css') }}' />
> 
> ausadmin.css isn't included in the patch, should it be ?

I haven't actually created it yet, I just threw it in there for future use. I can drop it for now, if you'd like.

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 605041 [details] [diff] [review]
rework html/javascript to make it easier to share across multiple pages; use WTForms

>diff --git a/auslib/web/templates/snippets/new_permission.html b/auslib/web/templates/snippets/new_permission.html

I was kinda-joking kinda-not about 'snippets' being a dirty word. If 'fragments' is fine by you then lets use that instead.

r+ with the nits on my previous comment.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #13)
> Comment on attachment 605041 [details] [diff] [review]
> rework html/javascript to make it easier to share across multiple pages; use
> WTForms
> 
> >diff --git a/auslib/web/templates/snippets/new_permission.html b/auslib/web/templates/snippets/new_permission.html
> 
> I was kinda-joking kinda-not about 'snippets' being a dirty word. If
> 'fragments' is fine by you then lets use that instead.

Absolutely. Let's keep the past in the past =).

And re: your IRC comment about maybe getting someone from webdev to look at it - I think that's a great idea. I'm going to land this anyways, but I'll get someone to look at what we've got so far, and address any feedback they have.

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 605041 [details] [diff] [review]
rework html/javascript to make it easier to share across multiple pages; use WTForms

https://jenkins.mozilla.org/job/Balrog/56/

Comment 16

[[e-mail] (:bhearsum)]

Rhelmer had a look at the current code over IRC yesterday and didn't find anything that needed fixing.