Closed Bug 672918 Opened 13 years ago Closed 13 years ago

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

Categories

(Release Engineering :: General, defect, P3)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: bhearsum)

References

Details

(Whiteboard: [ignore comment #0])

Attachments

(2 files, 1 obsolete file)

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.
I've been working on designing this here: https://wiki.mozilla.org/User:Bhearsum/AUS3/Administration
Assignee: nobody → bhearsum
Attached patch structure + /users implementation (obsolete) (deleted) — Splinter Review
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.
Attachment #556663 - Flags: feedback?(nrthomas)
Attachment #556663 - Flags: feedback?(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
Attachment #556663 - Flags: feedback?(catlee) → feedback+
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.
Attachment #556663 - Flags: feedback?(nrthomas) → feedback+
Whiteboard: [aus3]
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
Attachment #556663 - Attachment is obsolete: true
Attachment #564627 - Flags: review?(nrthomas)
Attachment #564627 - Flags: review?(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?
Attachment #564627 - Flags: review?(catlee) → review+
Attachment #564627 - Flags: review?(nrthomas) → review+
I just realized that this work was supposed to go in bug 672916 =\. Updating the summary to reflect what's actually gone on here.
Summary: create AUS3 API for build machines → create human admin interface for AUS3
Whiteboard: [aus3] → [aus3][ignore comment #0]
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.
Attachment #564627 - Flags: checked-in+
Summary: create human admin interface for AUS3 → create basic structure/code for balrog's human admin interface
Whiteboard: [aus3][ignore comment #0] → [ignore comment #0]
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").
Attachment #605041 - Flags: review?(nrthomas)
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 ?
(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 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.
Attachment #605041 - Flags: review?(nrthomas) → review+
(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 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/
Attachment #605041 - Flags: checked-in+
Rhelmer had a look at the current code over IRC yesterday and didn't find anything that needed fixing.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: