Bug739235 admin ui history view #6

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

peterbe commented Dec 12, 2012

@bhearsum r?

The only thing missing from this is that the tests have a bit of a low coverage.
For example, it only renders the Recent Changes table with an inserted permission, an updated permission and a deleted permission.

For example, it doesn't show an updated release or an updated rule.

Also, what I did was I made the Recent Changes table a view that renders just the <table> HTML chunk. You can see how it's used near the top of index.html with the .load() thing.

@mozbhearsum mozbhearsum commented on an outdated diff Dec 12, 2012

auslib/admin/base.py
@@ -41,4 +42,7 @@ def isa(error):
app.add_url_rule('/rules.html', view_func=RulesPageView.as_view('rules.html'))
app.add_url_rule('/rules', view_func=RulesAPIView.as_view('rules'))
app.add_url_rule('/rules/<rule_id>', view_func=SingleRuleView.as_view('setrule'))
+app.add_url_rule('/diff/<type_>/<change_id>/<field>', view_func=DiffView.as_view('diff.html'))
+app.add_url_rule('/view/<type_>/<change_id>/<field>', view_func=FieldView.as_view('field.html'))
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

Can you put these two endpoints under /history? They don't make sense in the context of the app as a whole, for me.

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/admin/views/history.py
+ pass
+
+
+class FieldView(AdminView):
+ """/view/:type/:id/:field"""
+
+ def get_value(self, type_, change_id, field):
+ tables = {
+ 'rule': db.rules,
+ 'permission': db.permissions,
+ 'release': db.releases,
+ }
+ try:
+ table = tables[type_]
+ except KeyError:
+ raise BadKeyError('Bad table')
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

What's the purpose behind catching this KeyError just to raise your own?

@peterbe

peterbe Dec 12, 2012

Contributor

The purpose was to be able to have better control over the error message. Perhaps I went overboard.
Basically, if you use /history/diff/NOT-A-TABLE/1/NOT-A-FIELD you get a 400 error because the parameters are used incorrectly. However, if you use a change_id that isn't found in the database, you get a 404 error.

I think I might just use a plain KeyError to make it feel simpler and lighter.

@mozbhearsum

mozbhearsum Dec 13, 2012

Member

Oh, I think it's great to get a 400 error in that case. It doesn't look like you need a special exception to do that though, since the 404 only happens on a ValueError. Is that right?

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/admin/views/index.py
return render_template('index.html', **data)
+
+
+class RecentChangesTableView(AdminView):
+ """/recent_changes_table.html
+
+ The reason for making this a view that returns just the <table> part
+ without wrapping in <html> or anything is so we can extend it to fetch
+ something like 10 more rows (where timestamp < latest last timestamp)
+ without having to refresh the page or having to use a paginator.
+
+ Also, by making the this table load async on the home page makes the
+ home page snappy and load quicker.
+ """
+ def get(self):
+ limit = 10
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

Should this be a query arg?

@peterbe

peterbe Dec 12, 2012

Contributor

Will do.

@peterbe

peterbe Dec 12, 2012

Contributor

Done.

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/admin/views/index.py
+
+ The reason for making this a view that returns just the <table> part
+ without wrapping in <html> or anything is so we can extend it to fetch
+ something like 10 more rows (where timestamp < latest last timestamp)
+ without having to refresh the page or having to use a paginator.
+
+ Also, by making the this table load async on the home page makes the
+ home page snappy and load quicker.
+ """
+ def get(self):
+ limit = 10
+ tables = {
+ 'rule': db.rules,
+ 'permission': db.permissions,
+ 'release': db.releases,
+ }
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

Can you move this lookup into a method inside db.py:AUSDatabase? It seems likely that other code may want to make use of it. Looks like it's already duplicated up in history.py, in fact!

@peterbe

peterbe Dec 12, 2012

Contributor

I would like to disagree. The only place it's needed is solely in the RecentChangesTableView view.
The day it's needed elsewhere we can move it and factor that out. This way, I like the "self-containment" of it exclusively to this view.

Also, the labels there "rule", "permission", "release" is quite exclusive to this view. We could after all freely change this to something more verbose like "Download Rule" instead of "rule" as this is the word that appears in the first column on the Recent Changes table.

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/admin/views/index.py
+ for x in each
+ if x not in primary_keys
+ )
+ changes[label][each['change_id']] = values
+
+ if len(recent_changes) >= limit:
+ # make sure this happens *after* `changes` is populated with
+ # the "next" change
+ break
+
+ change = None
+ if each['data_version'] > 1:
+ # easy, it's an update
+ change = 'update'
+ needs_diff[label].append(each['change_id'])
+ elif each['data_version'] == 1:
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

I hadn't realized that we could figure out the type of change just be looking at data_version - very nice!

@peterbe

peterbe Dec 12, 2012

Contributor

When I initially coded that I thought it was too simply to be true. I'm slowly getting over that :)

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/admin/views/index.py
return render_template('index.html', **data)
+
+
+class RecentChangesTableView(AdminView):
+ """/recent_changes_table.html
+
+ The reason for making this a view that returns just the <table> part
+ without wrapping in <html> or anything is so we can extend it to fetch
+ something like 10 more rows (where timestamp < latest last timestamp)
+ without having to refresh the page or having to use a paginator.
+
+ Also, by making the this table load async on the home page makes the
+ home page snappy and load quicker.
+ """
+ def get(self):
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

This method is pretty big, and some of the things it does (like figuring out the types of each change) seem like they belong in the History class or somewhere else more general purpose. For what's left, a comment at the start of each block would be helpful, too. What do you think?

@peterbe

peterbe Dec 12, 2012

Contributor

I'll take a stab at it.
I'm generally not afraid of large function bodies if there's no reason not to.
I think some pieces could be moved out into private methods or something just to make it feel/look more manageable.

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/admin/views/index.py
+ (k, prev_other_values[k])
+ for k in prev_other_values
+ if k not in _history_keys
+ ))
+
+ data = {
+ 'inserts': inserts,
+ 'diffs': diffs,
+ 'deletes': deletes,
+ 'recent_changes': recent_changes,
+ }
+
+ return render_template('fragments/recent_changes_table.html', **data)
+
+
+class PrinterFriendlyDict(dict):
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

This should go in a new file in auslib/util/

@peterbe

peterbe Dec 12, 2012

Contributor

I've now moved it and created test_util.py

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Dec 12, 2012

auslib/test/admin/views/test_history.py
@@ -0,0 +1,97 @@
+import json
+from auslib.admin.base import db
+from auslib.test.admin.views.base import ViewTest
+
+
+class TestHistoryView(ViewTest):
+
+ def testFieldViewBadValues(self):
+ url = '/history/view/notatable/1/whatever'
@mozbhearsum

mozbhearsum Dec 12, 2012

Member

Can you split up any of the tests with multiple different requests into multiple tests? I try to go by the one-condition, one-test philosophy, because it helps get a more whole view of what's broken. (Eg, if the first assert here fails we never test the other ones, so we don't know if they'll fail or not).

@peterbe

peterbe Dec 12, 2012

Contributor

Will do.

Member

mozbhearsum commented Dec 12, 2012

Based on my playing with it and looking at the code, I'm pretty happy with this. There's a couple of structural things and a few small nits to be addressed, I commented on those areas directly.

Contributor

peterbe commented Dec 12, 2012

I have (hopefully) addressed all nits.

The biggest change in peterbe/balrog@50be2df is that I refactored and tidied up stuff so that the get() method isn't 2 million lines in one block.

Instead I introduced a method called decorateChanges() which loops over the recent changes variable and fishes out what the diff/insert/removal was an attaches that.

I was having trouble reading this block at first until I realized that 'label' is the table name, and 'each' is a row in the history table. Can you rename these to 'table_name' and 'change' or something similar? You'll need to rename the local 'change' variable to 'change_type' or something similar, too. r=me with that changed.

Owner

peterbe replied Dec 13, 2012

I'll s/each/change but keep label since it's not the table name. It's just a label that is associated with that table. That's the label that is displayed in the first column on the Recent Changes table.

Contributor

peterbe commented Dec 13, 2012

Less each more change :)

Member

mozbhearsum commented Dec 17, 2012

This looks good to land to me.

Contributor

peterbe commented Dec 17, 2012

hurray! 303116b

peterbe closed this Dec 17, 2012

@JohanLorenzo JohanLorenzo pushed a commit to JohanLorenzo/balrog that referenced this pull request Sep 9, 2016

@davemo davemo [balrog-ui] Merge pull request #6 from testdouble/patch-1
Seems like >= is more appropriate so long as we commit to keeping this up to date.
d00d2dc

@JohanLorenzo JohanLorenzo pushed a commit to JohanLorenzo/balrog that referenced this pull request Sep 9, 2016

@gerva gerva [balrog-ui] Merge pull request #6 from petemoore/bug1076810
Bug 1076810 - RelEng CI tests should be available over the internet, add irc notifications for balrog-ui repository,r=mgerva
874e9f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment