734398 - Add balrog admin ui view for rules: release mapping, throttling

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Erick Dransch [:edransch]]

The Balrog admin UI should allow the user to change the mapping for a rule.
It should make some intelligent guesses at possible updates and offer them in a dropdown menu, with an option to type a mapping if none match.

A first step and the 'intelligent guesses' part could be to do some prefix matching with columns in the releases table and offer the options with the longest matching prefix & higher version numbers.

Comment 2

[Erick Dransch [:edransch]]

This bug now includes adding throttling control to the admin ui. Throttling control shares a view with the mappings, so it makes sense to keep it in the same bug.

Comment 3

[Erick Dransch [:edransch]]

Attachment: Add rules view

Add rules view with field for throttling, and a select field for mapping.
Currently the select field for mapping will show all releases, better ordering still to come.

Comment 4

[Erick Dransch [:edransch]]

Attachment: Screenshot of work-in-progress

Comment 5

[Erick Dransch [:edransch]]

Attachment: Screenshot of work-in-progress

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 605892 [details] [diff] [review]
Add rules view

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

::: auslib/db.py
@@ +556,5 @@
>  
> +    def getRuleById(self, rule_id, transaction=None):
> +        """ Returns the unique rule that matches the give rule_id """
> +        rules = self.select( where=[self.rule_id==rule_id], transaction=transaction)
> +        found = rules.__len__()

Use len(rules) instead of rules.__len__()

::: auslib/web/templates/snippets/rules.html
@@ +11,5 @@
> +    </tr>
> +    {% for row in rules %}
> +    <tr>
> +        {% set rule_id = row['rule_id'] %}
> +        {% for key,value in row.iteritems() %}

Similar thing here as in releases: you're subject to pseudorandom ordering of these columns that may change over time. Despite being a bit more verbose, I think it's better to be explicit here and ordering things in a more sensible order. Also like releases, rule_id and data_version should be hidden fields.

::: auslib/web/views/forms.py
@@ +36,4 @@
>              log.debug('JSONTextField: No value list, setting self.data to {}')
>              self.data = {}
>  
> +class DbEditableForm(Form):

It looks like you had some trouble getting the right diff, this hunk is part of bug 734083, right?

::: auslib/web/views/rules.py
@@ +21,5 @@
> +        forms = {}
> +
> +        for rule in rules:
> +            rule_id = rule['rule_id']
> +            rule_id_str = str(rule_id)

This only gets used once, you may as well do it inline rather than adding this variable for it. You can just call rule_id 'id', too, since it's obvious what it is in the context of this loop.

@@ +27,5 @@
> +                                    mapping=rule['mapping'], priority=rule['priority'], 
> +                                    data_version=rule['data_version'])
> +            forms[rule_id].mapping.choices = [(item['name'],item['name']) for item in 
> +                                                db.releases.getReleaseNames()]
> +        log.debug(forms)

What actually gets printed here? I'm concerned about it really clogging up logfiles.

@@ +31,5 @@
> +        log.debug(forms)
> +        return render_template('rules.html', rules=rules, forms=forms)
> +
> +class SingleRuleView(AdminView):
> +

Please add a docstring indicating which endpoint this is for.

Comment 7

[Erick Dransch [:edransch]]

Attachment: Updated Rules View patch

Addressed feedback and added tests for the rules view.
Still need to add a test to see what happens if the user tries to execute a post but does not have permissions to modify the database.

Comment 8

[Erick Dransch [:edransch]]

Attachment: Updated Rules View Patch

Added testing to make sure unauthorized post fails.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 606626 [details] [diff] [review]
Updated Rules View Patch

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

This is looking pretty good!

::: auslib/test/web/views/test_rules.py
@@ +26,5 @@
> +        ret = self._get('/rules.html')
> +        self.assertEquals(ret.status_code, 200)
> +        self.assertTrue('<option selected="selected" value="d">d</option>' in ret.data, msg=ret.data)
> +        self.assertTrue('<input id="1-throttle" name="1-throttle" type="text" value="71" />' in ret.data, msg=ret.data)
> +        self.assertTrue('<input id="1-priority" name="1-priority" type="text" value="73" />' in ret.data, msg=ret.data)

I see what you want to do here, but it's bad form to test multiple things in the same test. testPost is testing _only_ that you can successfully change data with a POST request. You've got an existing test for /rules.html, and if you want to test to make sure that priority and throttle show up OK, do it there, with the existing test data.

::: auslib/web/templates/snippets/rules.html
@@ +1,3 @@
> +{% if rules %}
> +Below is a complete list of all of the rules
> +{% endif %}

Same thing here as in releases.html, put everything inside of the if.

::: auslib/web/views/rules.py
@@ +10,5 @@
> +log = logging.getLogger(__name__)
> +
> +
> +# We should enforce one form per database row per page, so that we can keep the
> +# data_version consistent on the page by updating the form.

This comment doesn't seem to be associated with anything. If it's for RulesPageView, can you put it in the docstring instead?

@@ +31,5 @@
> +
> +class SingleRuleView(AdminView):
> +    """ /rules/<rule_id> """
> +
> +    # Changed_by is available via the requirelogin decorator

Nit: s/Changed/changed/

Comment 10

[Erick Dransch [:edransch]]

Attachment: Updated patch - rules view

Comment 11

[Erick Dransch [:edransch]]

Updated from meeting:

1) Logically group rules. Possibly sort them with decreasing priority when displaying.

2) Provide pagination, sorting by column, and arbitrary filtering of rules. (Use slavealloc as an example. What library is used there?)

3) Stick to in-table editing, with a single 'submit' button per row. We can move the editing to a separate page in the future if we decide that's better.

4) Perhaps add a filter to display rules by incoming request.

5) Add a tooltip or 'more info' button to display the rule_id, etc. (This simplifies debugging)

6) Need a method to add a new rule. Also need a method to copy a rule into a new rule, with modifications.

7) Replace 'None' with 'Any' in the table.

8) We should consider the possibility of having an Enable/Disable column in the Rules table. For now we will leave it off.

Comment 12

[Erick Dransch [:edransch]]

Attachment: Work in Progress

Updated patch which adds:
* Add New Rule form
* Form validation on the server after posting
* DataTables and Jquery-ui for displaying/editing rules

Still to come:
* Order Releases in select field dropdown in Rules view (requires new columns in the Releases table)
* Add more validation for the Rules forms (each field should be validated)
* Extend tests to exercise the new code
* Re-organize the javascript/html

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 613606 [details] [diff] [review]
Work in Progress

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

Overall I think this is looking good! Some comments are inline below.

::: auslib/db.py
@@ +376,1 @@
>          if self.history and not changed_by:

This file looks pretty much fine, but please add tests for all of the new methods you've created.

@@ +518,4 @@
>          if self._matchesRegex(ruleChannel, fallbackChannel):
>              return True
>  
> +    def insertRule(self, changed_by, what, transaction=None):

Naming nit: addRule (since we already use addRelease elsewhere).

@@ +519,5 @@
>              return True
>  
> +    def insertRule(self, changed_by, what, transaction=None):
> +        log.debug("insert rule")
> +        log.debug(what)

If these log messages will be kept, please add the standard Class.Method prefix to the output.

@@ +522,5 @@
> +        log.debug("insert rule")
> +        log.debug(what)
> +        ret = self.insert(changed_by=changed_by, transaction=transaction, **what)
> +        rule_id = ret.inserted_primary_key
> +        return rule_id

Nit: no need to assign to rule_id

@@ +574,5 @@
> +        rules = self.select( where=[self.rule_id==rule_id], transaction=transaction)
> +        found = len(rules)
> +        log.debug("Rules.getRuleById: Rules found: %s", found)
> +        if found > 1 or found == 0:
> +            return None

It should be impossible, but can you add some debug output for the found > 1 case? That seems like something we'd want to debug if it happens.

::: auslib/web/static/ausadmin.js
@@ +76,5 @@
> +
> +    throttle = $('[name='+rule_id+'-throttle]', ruleForm).val();
> +    data_version = $('[name='+rule_id+'-data_version]', ruleForm).val();
> +    mapping = $('[name='+rule_id+'-mapping]', ruleForm).val();
> +    priority = $('[name='+rule_id+'-priority]', ruleForm).val();

Seeing this pattern over and over again makes me think we should factor it out to a helper function. What do you think?

::: auslib/web/static/rules.js
@@ +1,1 @@
> +/* Create an array with the values of all the input boxes in a column */

I'll admit that I didn't read the contents of this very well, but it looked OK to me. A couple of nits though:
- Source the original where you copied the bulk of this from
- Comment your changes well

::: auslib/web/templates/base.html
@@ +7,2 @@
>  <script type='text/javascript' src='{{ url_for('static', filename='jquery-1.6.4.js') }}'></script>
> +<script type='text/javascript' src='{{ url_for('static', filename='DataTables-1.9.0/media/js/jquery.dataTables.min.js') }}'></script>

This file seems to be missing from the patch. Let's just name it "jquery.dataTables-1.9.0.min.js" though, no need for the long path.

::: auslib/web/templates/fragments/new_rule.html
@@ +19,5 @@
> +        <th>Update</th>
> +    </tr></thead>
> +    <tbody>
> +        <tr>
> +            {% set form = new_rule_form %}

You can just use new_rule_form directly, no need to create another variable for it.

@@ +66,5 @@
> +            <td id='header_arch_new'>
> +                {{ form.header_arch()|safe }}
> +            </td>
> +            <td id='Update_new'>
> +                {{ form.hidden_tag() }}

Nice find on hidden_tag()!

::: auslib/web/templates/fragments/rules.html
@@ +1,3 @@
> +{% if rules %}
> +Below is a complete list of all of the rules
> +<form id='rules_form' onsubmit='submitRuleForm($("#rules_form")); return false;'>

IIRC you told me that you can't use a <form> per rule because you can't put a <form> inside of a table - is that right?

::: auslib/web/templates/rules.html
@@ +1,4 @@
> +{% extends "base.html" %}
> +{% block title %}AUS3 Admin - Rules{% endblock %}
> +{% block head %}
> +<link rel='stylesheet' type='text/css' href='{{ url_for('static', filename='css/smoothness/jquery-ui-1.8.18.custom.css') }}' />

This file seems to be missing from the patch, too.

@@ +7,5 @@
> +    .ui-button-icon-only .ui-button-text { padding: 0.35em; } 
> +    .ui-autocomplete-input { margin: 0; padding: 0.48em 0 0.47em 0.45em; }
> +    .ui-autocomplete{ max-height:600px; overflow-y: auto; overflow-x: hidden;}
> +</style>
> +<script type='text/javascript' src='{{ url_for('static', filename='js/jquery-ui-1.8.18.custom.min.js') }}'></script>

This one too.

::: auslib/web/views/forms.py
@@ +78,5 @@
> +    build_target = TextField('Build Target')
> +    os_version = TextField('OS Version')
> +    dist_version = TextField('Dist Version')
> +    comment = TextField('Comment')
> +    update_type = TextField('Update Type', validators=[Required()] )

I like the version you pastebin'ed much better ;).

::: auslib/web/views/rules.py
@@ +13,5 @@
> +    """/rules.html"""
> +    # changed_by is available via the requirelogin decorator
> +    @requirelogin
> +    @requirepermission(options=[])
> +    def _post(self, transaction, changed_by):

This should be attached to a view for /rules, rather than rules.html.

@@ +18,5 @@
> +        # a Post here creates a new rule
> +        try:
> +            form = NewRuleForm()
> +            form.mapping.choices = [(item['name'],item['name']) for item in 
> +                                                db.releases.getReleaseNames()]

Please use retry() for this, and all of the other db operations too.

@@ +40,5 @@
> +                        header_arch = form.header_arch.data)
> +            rule_id = db.rules.insertRule(changed_by=changed_by, what=what, transaction=transaction)
> +            return Response(status=200, response=rule_id)
> +        except ValueError, e:
> +            return Response(status=400, response=e.args)

What ValueError is getting caught here?

@@ +71,5 @@
> +                                    update_type = rule['update_type'],
> +                                    header_arch = rule['headerArchitecture'],
> +                                    data_version=rule['data_version'])
> +            forms[_id].mapping.choices = [(item['name'],item['name']) for item in 
> +                                                db.releases.getReleaseNames()]

This is going to hit the DB once per rule. You can execute db.releases.getReleaseNames once at the start of this method, then use it in the new_rule_form and each iteration of this loop, instead.

@@ +79,5 @@
> +    """ /rules/<rule_id> """
> +
> +    def get(self, rule_id):
> +        rule = db.rules.getRuleById(rule_id);
> +        return render_template('single_rule.html', rule=rule)

You should return 404 if the rule doesn't exist. As it stands now, I think it'll return a rendered template with empty values. Also, I think this should use a fragment, not a full HTML page, because this is an API method instead of an HTML file. (I know you're planning to reorganize some HTML, just wanted to call this particular one out explicitly.)

@@ +113,5 @@
> +            log.debug("SingleRuleView: POST: old_data_version: %s", form.data_version.data)
> +            db.rules.updateRule(changed_by, rule_id, what, old_data_version=form.data_version.data, transaction=transaction)
> +            return Response(status=200)
> +        except ValueError, e:
> +            return Response(status=400, response=e.args)

Same thing here, what can potentially raise ValueError?

Comment 14

[Erick Dransch [:edransch]]

Attachment: Updated patch - address feedback

Comment 15

[Erick Dransch [:edransch]]

> ::: auslib/web/templates/fragments/rules.html
> @@ +1,3 @@
> > +{% if rules %}
> > +Below is a complete list of all of the rules
> > +<form id='rules_form' onsubmit='submitRuleForm($("#rules_form")); return false;'>
> 
> IIRC you told me that you can't use a <form> per rule because you can't put
> a <form> inside of a table - is that right?

Correct, it's either a form for the whole table or a form inside a <td> tag. One form per cell is no good for our purposes, so it had to be the whole table.


> @@ +113,5 @@
> > +            log.debug("SingleRuleView: POST: old_data_version: %s", form.data_version.data)
> > +            db.rules.updateRule(changed_by, rule_id, what, old_data_version=form.data_version.data, transaction=transaction)
> > +            return Response(status=200)
> > +        except ValueError, e:
> > +            return Response(status=400, response=e.args)
> 
> Same thing here, what can potentially raise ValueError?

I've removed that catch for now. I don't remember why I put it there. I'll keep an eye out for Errors that should be caught by this, and if I find any we can add it back.

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 614819 [details] [diff] [review]
Updated patch - address feedback

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

::: auslib/test/test_db.py
@@ +383,5 @@
> +                    update_type='z',
> +                    priority=60)
> +        rule_id = self.paths.addRule(changed_by='bill', what=what) 
> +        rule_id = rule_id[0]
> +        rule = self._stripNullColumns([self.paths.getRuleById(rule_id)])

You should be using self.paths.t.select() to do this, instead. I know it's a pain, but when running unit tests you shouldn't use code of yours that you aren't testing. Doing it this way makes sure that testAddRule can't fail when getRuleById breaks -- which can confuse things when you're debugging test failures. Similar thing in your other tests.

::: auslib/web/static/rules.js
@@ +200,5 @@
> +    console.log(data);
> +    $.ajax(url, {'type': 'post', 'data': data})
> +    .error(handleError
> +    ).success(function(data) {
> +        window.location = getRuleUrl(data);

Per IRL conversation, this shouldn't redirect to a new page - it should add the rule to the form instead. You should be GET'ing the new rule HTML from /rules/:id, and appending it to the form. The permissions UI has a good example of how to do this.

::: auslib/web/views/rules.py
@@ +23,5 @@
> +        releaseNames = retry(db.releases.getReleaseNames, sleeptime=5, retry_exceptions=(SQLAlchemyError,))
> +        form.mapping.choices = [(item['name'],item['name']) for item in releaseNames]
> +        form.mapping.choices.insert(0, ('', 'NULL' ) )
> +        if not form.validate():
> +            log.debug(form.errors)

Please preface this with a log.debug("auslib.web.views.rules.RulesPageView: ...") message.

@@ +44,5 @@
> +        rule_id = retry(db.rules.addRule, sleeptime=5, retry_exceptions=(SQLAlchemyError,),
> +                kwargs=dict(changed_by=changed_by, what=what, transaction=transaction))
> +        return Response(status=200, response=rule_id)
> +
> +    def get(self):

This returns an entire HTML page, and should remain in the rules.html view.

Comment 17

[Erick Dransch [:edransch]]

Attachment: Updated patch - address feedback

Updated to address latest feedback.
I've also included the external libraries in the patch this time.

Comment 18

[[e-mail] (:bhearsum)]

Comment on attachment 615732 [details] [diff] [review]
Updated patch - address feedback

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

If you can address just the few things I note below (eg, no other new code) we can get this landed as a baseline, which will make future reviews (and hopefully patch management) easier.

::: auslib/test/web/views/test_rules.py
@@ +4,5 @@
> +from auslib.test.web.views.base import ViewTest, JSONTestMixin, HTMLTestMixin
> +
> +class TestRulesAPI_JSON(ViewTest, HTMLTestMixin):
> +    def testGetRules(self):
> +        ret = self._get('/rules.html')

This test should go in its own class now, since we've been using test class per view to organize.

::: auslib/web/static/rules.js
@@ +25,5 @@
> +        $('#rules_table').dataTable({
> +            "aoColumnDefs": [
> +                { "sSortDataType": "dom-select", "aTargets":[0] },
> +                { "sSortDataType": "dom-text", "aTargets":[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15] },
> +                { "sType": "numeric", "aTargets": [1, 2] }

Can you add comments here, indicating that the numbers refer to columns? It's not obvious at first.

@@ +159,5 @@
> +    return SCRIPT_ROOT + '/rules';
> +}
> +function getBaseRuleUrl() {
> +    return SCRIPT_ROOT + '/rules.html';
> +}

I don't think this function is used anymore.

@@ +208,5 @@
> +        .error(handleError)
> +        .success(function(data) {
> +            console.log("Got rule:");
> +            table.append(data);
> +            table.dataTable().fnDraw();

Looks like this turned out to be pretty easy, woo!

::: auslib/web/templates/rules.html
@@ +1,1 @@
> +{% extends "base.html" %}

This file, and everything it includes, look good to me now. Thanks for taking the time to organize it well!

Comment 19

[Erick Dransch [:edransch]]

Attachment: Updated patch - address feedback

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 615786 [details] [diff] [review]
Updated patch - address feedback

Landed, Jenkins run is here: https://jenkins.mozilla.org/job/Balrog/78/