929555 - balrog's rules page is excessively wide

Status: RESOLVED WONTFIX

(Release Engineering Graveyard :: Applications: Balrog (frontend), defect)

Description

[[e-mail] (:bhearsum)]

Right now, the rules page is simply a table showing all of the columns in the rules table. On my screen, it's about 2.5 screens wide - which is way too big to be usable.

We had a similar problem on Ship It which was improved by using more vertical space and some better fonts. It's a little different on Balrog because any rule can be edited (as opposed to Ship It, where most releases are just there for historical purposes), but I'm sure there's something that can be done here.

Massimo, can you take a look at this when you have time? We should probably chat before you dive in too far so I can give you a quick tour of the code and web interface.

Comment 1

[[e-mail] (:bhearsum)]

mass component change

Comment 2

[Massimo Gervasini [:massimo]]

Attachment: rules page

Here is a view of the rules page as it is now

Comment 3

[Massimo Gervasini [:massimo]]

Attachment: overview.png

...and this is the zoom out of the table so it can fit in the screen

Comment 4

[Massimo Gervasini [:massimo]]

Attachment: rules_table.png

Hi Ben,

I think I might have found how to make the table size human here's an example using datables hidden rows (http://www.datatables.net/release-datatables/examples/api/row_details.html) the attached email is a screenshot of a semi-working prototype

If we can find 6-7 important columns, we can arrange everything else in a "detail" panel. 

This is just an idea and I am sorry for the broken css/images, but I'd like to hear your opinion before spending too much time on the graphic layout.

Comment 5

[[e-mail] (:bhearsum)]

This looks like a big overall improvement to me, with a few caveats:
* There's no need to duplicate fields like "Product" in the drop down
* Will the filter box look at hidden fields, or just the non-hidden ones?
* I'd suggest moving "Version" to the hidden section and "Channel" to the non-hidden section. Product/Channel/Priority are generally the most important fields when it comes to evaluating a rule.
* If there's room, unhiding "Comment" would be good too. It would make rules like our OS blocking ones less confusing at a glance.
* How do you feel about moving the Update/Clone/etc. buttons into the hidden section? I'm not sure if they're necessary on the primary UI or not.

Nick may have thoughts here too.

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Massimo asked me to have a look at this, so here's some ideas. Note that I am not completely sure how this tool works, so take it with a grain of salt ;)

- There seem to be many fields that only ever contain numbers (like priority etc). Those could be smaller. Same goes for locale-fields.
- Some fields like »Product« could probably also become dropdown menus. That would help structure the table and make it easier to scan.
- It might help to style the text fields like normal text (no box around it) by default. The hover and focused state could then show the box. That would reduce the visual noise quite a bit (and perhaps also allow for narrower columns.
- If fields can be grouped in any way, that might help as well.
- The buttons at the end of the rows could be closer together or even grouped into one column

I hope those ideas help a little!

Comment 7

[Massimo Gervasini [:massimo]]

Attachment: 929555.patch

Thanks Philipp and Ben, for the feedback

This is a work in progress so it do not expect it to work perfectly. 

new features:
* add a new rule and view rule are in different tabs
* rules table now fits in a single screen
* all the rules table columns are searchable
* buttons: edit, clone, delete and revisions are now in the details row
* minor fixes to buttons, navbar,...
* added bootstrap datatables plug in 

to do:
* implement "edit" and "clone"
* fix "delete"
* fix "add new rule" page
* re-enable the comboboxes everywhere
* there's a lot of code that can be removed in rules.js

To make the table fit in a single screen, all the input elements have been removed. If users want to change a rule, they have to click on "edit" (this feature is not implemented yet) - using the same mechanism we use in ship it to edit a submitted release.
The reason behind this change is that input columns are using a lot of space and they make the detail row more complex. 

Ben does this work for you? I know that having an editable rules page is very useful but it requires more javascript code and more time to be finished.

Comment 8

[[e-mail] (:bhearsum)]

This looks like a good improvement overall, but there's a few UI/UX things I'd like to see addressed:
* Viewing and editing existing rules is the most common interaction, so we should default to the view/edit page. It would also be great to make "priority" the default sort (descending).
* I don't think it's a good idea to force the user onto a separate page to edit a rule -- it's sometimes helpful to be able to see other rules while you are editing. Is there any reason you can't put an editable form in the expanded version of the rule?
* "token" (I assume that's the CSRF token?) doesn't need to be viewable
* Adding data_version to the expanded rule would be great. It shouldn't be editable, just there for information purposes.
* "clone" hasn't really ended up being useful, it can probably go away rather than you spending time editing it.

I also want to mention that you should feel free to change the organization of the templates and JS as much as you see fit -- they're pretty confusing right now.

Thanks so much for your work on this Massimo, this is looking much better already!

Nick, do you have anything you'd like to add?

Comment 9

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

Wow, that is so much nicer to look at. Some suggestions:
* some indication of non-empty hidden fields would be helpful (eg a buildID restriction isn't visible until you hit the blue button)
* some of the columns are large because of their headers but have small values (eg Background Rate). We could try abbeviations like 'Back.\nRate', with a hover for the full name. Getting a dump of the current production rules would probably help set useful widths
* in addition to Priority sort, I would then sort on Product, and then Channel

Comment 10

[Massimo Gervasini [:massimo]]

Attachment: [balrog] added horizontal form, removed clone button.patch

Thanks Ben and Nick for the feedback.

This is a work in progress. I have started to fix the problems you have found: I have removed clone button and added a form-horizontal in the details panel but it's very fragile, I have found a couple of lucky numbers for the label and input size that make it look "decent" but the vertical space between elements is wrong. I think there is something wrong with the css having a form-group element inside the javascript generated tr > td but I did find how to fix this. Any idea?

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 8395209 [details] [diff] [review]
[balrog] added horizontal form, removed clone button.patch

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

(In reply to Massimo Gervasini [:mgerva] from comment #10)
> Created attachment 8395209 [details] [diff] [review]
> [balrog] added horizontal form, removed clone button.patch
> 
> Thanks Ben and Nick for the feedback.
> 
> This is a work in progress. I have started to fix the problems you have
> found: I have removed clone button and added a form-horizontal in the
> details panel but it's very fragile, I have found a couple of lucky numbers
> for the label and input size that make it look "decent" but the vertical
> space between elements is wrong. I think there is something wrong with the
> css having a form-group element inside the javascript generated tr > td but
> I did find how to fix this. Any idea?

I don't have any ideas, this is far beyond my area of expertise :(. #webdev is very friendly though, they've helped with similar problems in the past.

Comment 12

[Massimo Gervasini [:massimo]]

Attachment: [balrog] Bug 929555 - updated bootstrap and jquery libs to lastest versions.patch

Hi Nick, Ben,

Working on the huge table, I have collected a lot of minor changes that we could start to land. This patch updates bootstap and jquery to the latest available versions and includes only minor fixes for the user interface: buttons, forms, tables, navbar, ... 
Changes are only at graphical level.

Hopefully, next patches will be smaller and totally focused on the rules page.

I am preparing a balrog demo instance with this patch on dev-master1.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 8404593 [details] [diff] [review]
[balrog] Bug 929555 - updated bootstrap and jquery libs to lastest versions.patch

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

Seems okay to me. It's probably good if this gets landed so that your next patches are smaller and easier to review.

Comment 14

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

Comment on attachment 8404593 [details] [diff] [review]
[balrog] Bug 929555 - updated bootstrap and jquery libs to lastest versions.patch

Deferring to Ben's review for this web stuff.

Comment 15

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

Attachment: Temporary workaround

Started playing with the width of the autocomplete widget to get rid of the truncation, then shrank the other input boxes and the minor/major combobox too. With this and some text (de)zoom you can see the whole page and still have legible text. Meant as a temporary hack while we work on a better solution.

Comment 16

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/f3fb727703b4cb0c5c5bcedbd828480916961399
Bug 929555, temporary workaround for rules page being excessively wide, r=bhearsum

Comment 17

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

Deployed by bug 1020490.

Comment 18

[Massimo Gervasini [:massimo]]

Attachment: balrog.patch

Hi all,

I have deployed this patch on dev-master1, you can access it here: http://dev-master1.srv.releng.scl3.mozilla.com:12000 (requires VPN access)

credentials: username/password is your email address

The main changes in this version are:
* rules can be added and edited.
* mappings column suggestions are populated dynamically

There is still some work to to do and things to be fixed as:
* mappings column is not sortable
* some elements on the edit pages have a wrong css (search box and next/previous)
* sometime alertify show huge messages
* mappings suggestions do not accept key up/key down
* details elements should be hidden by default in add rule page

and I guess there are many others things to do. 

Nick and Ben, could you give this version a try and give me a feedback on it?

Thanks

Comment 19

[[e-mail] (:bhearsum)]

Comment on attachment 8470246 [details] [diff] [review]
balrog.patch

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

This looks fantastic. It's worth noting that Peter from webdev will be working on a big overhaul to the UI in Q4, but this is still very nice to have in the meantime. If it doesn't require a ton of time to finish up, I'd love to have it done and landed. If it's going to take a whole week or something though, maybe it's not worth it. I'm not going to be as fussy about the UI code because of this, too.

Some overall stuff:
* Page load times are much improved - I love it.
* Autocomplete of the mappings seems to mostly work, but there's a couple of problems:
** It talks to the server with every keystroke - I think it would be better if it could wait for a slightly delay - maybe 500ms? That would greatly reduce the number of roundtrips.
** I'm often getting repeat entries in the autocomplete. It seems to build up every time. Maybe you need to check for dupes before adding something?
* "Version Data" should be "Data Version".
* It's great that there's a drop down for Product, but there needs to be a way to enter a custom one too - there's no guarantee that those are the only three we'll ever have.
* The edit/delete buttons should be styled differently than "revision" - they cause a change to be made, whereas revision is just a link to another page.
* "revision" would be better called "Past Revisions", while you're poking around there.
* Error messages are confusing. Eg: "rule_14 balrog is not allowed to alter rules that affect Thunderbird has a wrong value!" - it looks like the "has a wrong value part" is being appended in JS. I think it's probably better to only show the error sent by the server. If it's currently not good enough, it should be changed to be more descriptive/informative.
* Errors when creating a rule do not show up anywhere. I can see requests happening in debugging console of Firefox, but nothing in the UI tells me it even attempted a request.
* Deleting rules doesn't work at all. It complains about data version and the CSRF token.

::: auslib/admin/base.py
@@ +63,5 @@
>  app.add_url_rule('/history/view/<type_>/<change_id>/<field>', view_func=FieldView.as_view('field'))
>  app.add_url_rule('/recent_changes_table.html', view_func=RecentChangesTableView.as_view(''))
>  app.add_url_rule('/', view_func=IndexPageView.as_view('index.html'))
> +app.add_url_rule('/mappings', view_func=MappingsView.as_view('mappings'))
> +app.add_url_rule('/mappings/<name>', view_func=CompletePartialMapping.as_view('mappings'))

A mapping and a release are the same thing - these should get integrated with /releases. You might need a slightly different name for the second one, because we already have /releases/<release>.

::: auslib/admin/views/rules.py
@@ +98,5 @@
> +class MappingsView(AdminView):
> +    """/mappings"""
> +    def get(self):
> +        mappings = [m['name'] for m in dbo.releases.getNames(limit=20)]
> +        return jsonify({'mappings': mappings})

I'm not quite sure I understand the point of this. Are there many cases where the first 20 release names are going to be very helpful?

@@ +104,5 @@
> +class CompletePartialMapping(AdminView):
> +    """/mappings/<name>"""
> +    def get(self, name):
> +        mappings = [m['name'] for m in dbo.releases.completePartialMapping(name, limit=20)]
> +        return jsonify({'mappings': mappings})

This is very cool, and using it (instead of loading all mappings at once) seems to have greatly improved the page load time.

::: auslib/db.py
@@ +949,4 @@
>          self.delete(changed_by=changed_by, where=where, old_data_version=old_data_version, transaction=transaction)
>  
> +    def getNames(self, limit=10):
> +        return self.select(columns=[self.name], distinct=True, limit=limit)

This method is unnecessary, you can use getReleaseInfo instead - just pass nameOnly=True.

@@ +951,5 @@
> +    def getNames(self, limit=10):
> +        return self.select(columns=[self.name], distinct=True, limit=limit)
> +
> +
> +    def completePartialMapping(self, name, limit=10):

Naming nit: "complete" is a verb and a noun, I think it would be better to call this something like "getReleasesLike(self, name, limit=10)"

Comment 20

[Massimo Gervasini [:massimo]]

We are moving to the new Balrog UI, closing this.