Closed Bug 696446 Opened 13 years ago Closed 13 years ago

[homepage] new shipping snippet for the team page

Categories

(Webtools Graveyard :: Elmo, defect, P2)

Product:
Webtools Graveyard
Component:
Elmo
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: peterbe)

References

(
URL
)

Details

(Whiteboard: [wireframes])

Attachments

(2 files, 1 obsolete file)

WIP patch
15.04 KB, patch
Details | Diff | Splinter Review
Fixes from review
31.14 KB, patch
Pike
: review+
Details | Diff | Splinter Review 
Separating out the part of bug 665646 which is about the shipping snippet.  See there.
Assignee: nobody → stas
Attached patch WIP patch — — Splinter Review 
Axel, can you take a look at this WIP patch and share your thoughts?  The biggest missing piece is the suggested signoff column.  I'd like to move most of the logic there to shipping.api, but I don't really like the code I end up with, yet.

Here's my branch on github:
https://github.com/stasm/elmo/tree/feature/bug-696446-teampage-shipping-snippet

And this is the same diff as in the patch, but on github:
https://github.com/stasm/elmo/compare/develop...feature%2Fbug-696446-teampage-shipping-snippet

FTR, the icons come from http://openiconlibrary.sourceforge.net/ and are GPLed.
Attachment #569561 - Flags: feedback?(l10n)
You can preview the patch on my dev server: http://elmo.stasmade.com/teams/nb-NO
First of all: What an awesome upgrade this is going to be! So much prettier than anything we had before. It will make more people contribute!

My thoughts are mainly visual and very much in mind for the first or second-time users:

* The title of the big table is "Applications & Signoffs" but it's not a list of applications. Consider just changing it to "Active Signoffs" or just "Signoffs" or something...
By the way, the ampersand needs to be & not just &.

* The odd/even shading doesn't make sense. The blame for that is on the weird behavior that sometimes a run has an appversion in bold and sometimes it doesn't. If that was made uniform it would make sense to use odd/even background shading. 

* I would love to see a row of heads on the table. For one thing, it would be make it possible to write the head "# strings" just ones instead of repeating this word for every row. 

* On the missing strings column why does it have a magnifying glass AND a text link? And why is the background ping and the text white? Consider removing the magnifying glass since it doesn't add any value. 

* Regarding the accepted, rejected, pending signoff icons, I would probably prefer if that was instead all three icons shown all the time but with a 0.1 opacity. The runs that have signoffs gets a 1.0 opacity green check, etc. The void of blank white space doesn't make sense and makes it confusing.
What Peter said, too.

I'd not go for just "Signoffs" as title, though. I think "Applications" is technically wrong, but it's the thing that's close to localizers. Being more precise risks being too jargon-y, IMHO.

One thing I wondered: The missing strings and signoffs are the really actionable data in that table. I'm not sure where they get more focus, on the left, or on the right border. I'd love to see the two in comparison, can be just a static html dump?
(In reply to Peter Bengtsson [:peterbe] from comment #4)
> First of all: What an awesome upgrade this is going to be! So much prettier
> than anything we had before. It will make more people contribute!

:)

I know, right?!
 
> * The title of the big table is "Applications & Signoffs" but it's not a
> list of applications. Consider just changing it to "Active Signoffs" or just
> "Signoffs" or something...
> By the way, the ampersand needs to be & not just &.

The right title would be "Latest runs on trees with signoff summary".  That's obviously too jargon, as Axel suggests.  "Applications" seems to convey the right message in a more friendly manner (even if not really correct).

> * The odd/even shading doesn't make sense. The blame for that is on the
> weird behavior that sometimes a run has an appversion in bold and sometimes
> it doesn't. If that was made uniform it would make sense to use odd/even
> background shading. 

The shading is supposed to help your eyes go from the left to the right.  How is it related to appversion names at all?

> * I would love to see a row of heads on the table. For one thing, it would
> be make it possible to write the head "# strings" just ones instead of
> repeating this word for every row. 

I was precisely trying to avoid this :)  I (and it seems like chowse as well) wanted to make it so you don't have to look anywhere else than the row you're interested in.  I should add a legend somewhere, true, but not a row of <th>s.

I also had another idea: a logged in user should be able to hide specific rows that are of no interest to her (with an ability to unhide all).  I'll file a follow up bug later.

> * On the missing strings column why does it have a magnifying glass AND a
> text link? And why is the background ping and the text white? Consider
> removing the magnifying glass since it doesn't add any value. 

I'm using icons to convey the status elsewhere.  Also, I was hoping that the magnifying glass will encourage an action, as in, come on in to see more details, investigate.  I can remove it, but I quite like it, tbh.

> * Regarding the accepted, rejected, pending signoff icons, I would probably
> prefer if that was instead all three icons shown all the time but with a 0.1
> opacity. The runs that have signoffs gets a 1.0 opacity green check, etc.
> The void of blank white space doesn't make sense and makes it confusing.

Cool idea, I'll experiment with it.


(In reply to Axel Hecht [:Pike] from comment #5)
> One thing I wondered: The missing strings and signoffs are the really
> actionable data in that table. I'm not sure where they get more focus, on
> the left, or on the right border. I'd love to see the two in comparison, can
> be just a static html dump?

Not sure if I get that.  Mind drafting that in ascii or something?  What would be the order of columns exactly?  Happy to create some static html for you to compare the two approaches, but I first need to understand what to change :)
(In reply to Staś Małolepszy :stas from comment #2)
> You can preview the patch on my dev server:
> http://elmo.stasmade.com/teams/nb-NO

Just some comments from the sidelines:
1) From just looking at it, I have no idea what the space or symbols on the right mean, I can only guess.
2) I found out that I could mouse over those icons and get a tooltip, which brought me into the light about what they mean - but only because I was on the desktop - on my tablet, I would have been out of luck, having no way to mouse over.
3) I have no idea what "5207 strings" means: how many have been translated, how many that tree has, or what?
4) I'm missing seeing obsolete strings and warnings or something that warns me that there are no sign-offs even though one could do one.

And I personally am not a big friend of table-like displays without marked columns of any kind - but that might just be a matter of taste.
(In reply to Staś Małolepszy :stas from comment #6)
> (In reply to Peter Bengtsson [:peterbe] from comment #4)

>  
> > * The title of the big table is "Applications & Signoffs" but it's not a
> > list of applications. Consider just changing it to "Active Signoffs" or just
> > "Signoffs" or something...
> > By the way, the ampersand needs to be &amp; not just &.
> 
> The right title would be "Latest runs on trees with signoff summary". 
> That's obviously too jargon, as Axel suggests.  "Applications" seems to
> convey the right message in a more friendly manner (even if not really
> correct).
> 

Hardcode Elmo users will understand the word "Appversions" but people who peek in, and potentially interested in joining-in, a "dumber" version like "Applications" will work better. 


> > * The odd/even shading doesn't make sense. The blame for that is on the
> > weird behavior that sometimes a run has an appversion in bold and sometimes
> > it doesn't. If that was made uniform it would make sense to use odd/even
> > background shading. 
> 
> The shading is supposed to help your eyes go from the left to the right. 

I guess my point is that it doesn't help. Because each row is so different (some have bold appversions some have just the tree code) the shading is confused for grouping. 

> How is it related to appversion names at all?
Isn't the name is bold the appversion name? E.g. "Fennec Aurora"

    {% if run.appversion %}
        <strong>{{ run.appversion }}</strong><br>
    {% endif %}

> 
> > * I would love to see a row of heads on the table. For one thing, it would
> > be make it possible to write the head "# strings" just ones instead of
> > repeating this word for every row. 
> 
> I was precisely trying to avoid this :)  I (and it seems like chowse as
> well) wanted to make it so you don't have to look anywhere else than the row
> you're interested in.  I should add a legend somewhere, true, but not a row
> of <th>s.
> 
It's a noble goal. Ideally it shouldn't be needed. 
But because a lot of things in the table isn't dead-obvious it might actually help. Like, what's in the first column? Trees or Appversions? What's in the second column? Strings total or strings missing?

> I also had another idea: a logged in user should be able to hide specific
> rows that are of no interest to her (with an ability to unhide all).  I'll
> file a follow up bug later.
> 
That is potentially dangerous. It would introduce more noisy options and buttons and people who just do Fennec might miss out the subtle hint that Thunderbird is needing some TLC. 

> > * On the missing strings column why does it have a magnifying glass AND a
> > text link? And why is the background ping and the text white? Consider
> > removing the magnifying glass since it doesn't add any value. 
> 
> I'm using icons to convey the status elsewhere.  Also, I was hoping that the
> magnifying glass will encourage an action, as in, come on in to see more
> details, investigate.  I can remove it, but I quite like it, tbh.
> 
The text label is so useful and complete that it makes you wonder what the magnifying glass contributes, if any. 

> > * Regarding the accepted, rejected, pending signoff icons, I would probably
> > prefer if that was instead all three icons shown all the time but with a 0.1
> > opacity. The runs that have signoffs gets a 1.0 opacity green check, etc.
> > The void of blank white space doesn't make sense and makes it confusing.
> 
> Cool idea, I'll experiment with it.
> 
It bothers me a bit that some rows have no icons. What does that mean?! No signoffs at all for that tree? Perhaps consider an icon just for that. 

> 
> (In reply to Axel Hecht [:Pike] from comment #5)
> > One thing I wondered: The missing strings and signoffs are the really
> > actionable data in that table. I'm not sure where they get more focus, on
> > the left, or on the right border. I'd love to see the two in comparison, can
> > be just a static html dump?
> 
> Not sure if I get that.  Mind drafting that in ascii or something?  What
> would be the order of columns exactly?  Happy to create some static html for
> you to compare the two approaches, but I first need to understand what to
> change :)

I like that stuff to appear on the right-hand side. It's such a common practice to put labels on the left and icons on the right that no matter what is most important it would jar most if it doesn't follow convention.
I think we also want a legend for the icons, and not just tooltips.

Alternative idea: I wonder if we can have something button-ish for sign-off (not really a button, probably) with the various icons inside. Like

/-----\
| X O |
\-----/

Re the row discoverability: We shouldn't rely on consecutive rows having appversions and not, that seems more coincidence. Also, we might even want to change the order :-/
Hello! After talking with Peter and Axel this afternoon, I've come up with some suggestions for the localization team page. Considering my VERY limited understanding of the needs of localizers, please take all recommendations with a grain of salt. Additionally, without localizer use cases that I can use to inform decisions, my suggestions here will all be pretty superficial. Hopefully later we can invest some time and develop proper use cases that can be used to inform wireframes.

- It seems to me like organizing the trees according to product ( as in http://people.mozilla.com/~pbengtsson/bikeshedding-teams-page/02/nb-NO.html ) makes the most sense, especially if the product sections are organized such that trees without signoffs are located at the bottom. I chose this method because everyone at all levels of experience understand products, and it does not hurt advanced users who want to know the tree name (it's still there, where they would expect it to be).

- I would sort the products by the most popular ones. Just so the one that most localizers will be using is on top, etc.

- Add column names: you'd be surprised how much this helps users in understanding the table

- I haven't heard the number of strings get mentioned as important, so I would suggest removing it from this table. 

- It seems like the important information in the "% Changed" graph isn't actually how much has been changed, but how much has not been looked at yet - the red part. Because of that, I'd suggest a few things:
	- Combine the grey and green areas, since they have both been attended to.  I would probably change the green to a light grey, since red/green colorblind users wouldn't be able to tell the difference between green and red.
	- Move the Red part to the end, since it is the part that is yet to be completed.
	- I would combine the column with the "## Missing" column, since that's really what the graph is about anyways.

- Finally, I'd add some labels to the icons. It's really tough to tell what they mean if you're not deeply ingrained in the system.

Here's how it looks all together :) 

http://cl.ly/0r2M0G2M0C3r3t1i1V0p

Of course, feel free to implement as many or as few of these suggestions as you would like to, they're just options for you to consider. Hopefully we can do a more thorough refresh later. Thanks!
I forgot to mention that it's worthwhile to make any actionable items look like buttons or links, as you can see from the mockup.
(In reply to Jason Grlicky [:grlicky] from comment #10)

> http://cl.ly/0r2M0G2M0C3r3t1i1V0p

This is so rad!
Attachment #569561 - Flags: feedback?(l10n)
Assignee: stas → peterbe
Remember, 90% of this is Stas work. I just tied it together with some nit fixes and the implementing of Jasons HTML. 

Also, I made a small improvement to the bugsy views and template so that the bugzilla URL can be properly urlencoded and not cause HTML validation errors.
Attachment #574482 - Flags: review?(l10n)
I appreciate that it might be hard to grasp the context of this patch so I pushed the branch to here: https://github.com/peterbe/elmo/tree/feature/bug-696446-teampage-shipping-snippet
I checked out your branch at http://elmo.stasmade.com/teams/nb-NO.  This looks promising :) A few comments:

- Consider not using the underline for the links to the compare-locales runs.

- I think I preferred the grouping from Jason's mockup where there was just one table and the application names would span a few tree rows.

- I had to pip install BeautifulSoup to make the branch work, you might need to add it to requirements and vendor-local.

Unralated to the actual code, I wonder how you got the commits from my branch into yours?  The network view suggest that you didn't branch off of my branch: https://github.com/peterbe/elmo/network which made it a bit hard for me to pull and merge your changes.
(In reply to Staś Małolepszy :stas from comment #15)
> I checked out your branch at http://elmo.stasmade.com/teams/nb-NO.  This
> looks promising :) A few comments:
> 
> - Consider not using the underline for the links to the compare-locales runs.
> 
Fair point. I just thought we'd stick with underlines because that's what we use for all of the other links on the site. 

> - I think I preferred the grouping from Jason's mockup where there was just
> one table and the application names would span a few tree rows.
> 
Also very fair point. However, can we hold that thought for a moment whilst we implement the new design. It can be an iteration for later. 

> - I had to pip install BeautifulSoup to make the branch work, you might need
> to add it to requirements and vendor-local.
> 
Whaaaaa! 
I can see that vendor-local/lib/python/html5lib/treebuilders/soup.py imports BeautifulSoup. However, I don't have it installed on my environment and mine's working just fine.
Have you got the traceback where it stumbled on the importerror?

> Unralated to the actual code, I wonder how you got the commits from my
> branch into yours?  The network view suggest that you didn't branch off of
> my branch: https://github.com/peterbe/elmo/network which made it a bit hard
> for me to pull and merge your changes.

Uh? Did I do it wrong? I used git fetch and then something like git checkout stasm/bug-....
(In reply to Peter Bengtsson [:peterbe] from comment #16)

> > - I had to pip install BeautifulSoup to make the branch work, you might need
> > to add it to requirements and vendor-local.
> > 
> Whaaaaa! 
> I can see that vendor-local/lib/python/html5lib/treebuilders/soup.py imports
> BeautifulSoup. However, I don't have it installed on my environment and
> mine's working just fine.
> Have you got the traceback where it stumbled on the importerror?

Are you checking using manage runserver?  It works fine then, but when you switch to wsgi, the compressor kicks in and gives me this:

Environment:


Request Method: GET
Request URL: http://elmo.stasmade.com/teams/nb-NO

Django Version: 1.3
Python Version: 2.7.2
Installed Applications:
['commons',
 'tower',
 'nashvegas',
 'django_arecibo',
 'compressor',
 'elmo',
 'commonware.response.cookies',
 'djcelery',
 'django_nose',
 'django.contrib.auth',
 'django_sha2',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.staticfiles',
 'django.contrib.admin',
 'product_details',
 'accounts',
 'homepage',
 'privacy',
 'life',
 'mbdb',
 'pushes',
 'dashtags',
 'l10nstats',
 'tinder',
 'shipping',
 'bugsy',
 'webby']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'commonware.middleware.FrameOptionsHeader',
 'commonware.middleware.HidePasswordOnException',
 'django_arecibo.middleware.AreciboMiddleware')


Template error:
In template /srv/http/elmo.stasmade.com/elmo/templates/base.html, error at line 46
   Caught ImproperlyConfigured while rendering: Error while importing lxml: No module named BeautifulSoup
   36 :    -
   37 :    - ***** END LICENSE BLOCK *****
   38 : {% endcomment %} -->
   39 : 
   40 : {% load compress %}
   41 : <html lang="{{ LANG }}" dir="{{ DIR }}">
   42 :   <head>
   43 :     <meta charset="utf-8">
   44 :     <link rel="shortcut icon" href="{{ STATIC_URL }}img/favicon.ico" type="image/x-icon">
   45 :     <link rel="icon" href="{{ STATIC_URL }}img/favicon.ico" type="image/x-icon">


   46 :      {% compress css %} 


   47 :     <link rel="stylesheet" href="{{ STATIC_URL }}css/style.css" type="text/css">
   48 :     {% endcompress %}
   49 : <title>{% block title_matter %}{% endblock %}</title>
   50 : {% block head_matter %}{% endblock %}
   51 : </head>
   52 : 
   53 : <body>
   54 : <div id="menu">
   55 :  <div id="auth">
   56 :    {% include "accounts/user_forms.html" %}


Traceback:
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/core/handlers/base.py" in get_response
  111.                         response = callback(request, *callback_args, **callback_kwargs)
File "/srv/http/elmo.stasmade.com/elmo/apps/homepage/views.py" in locale_team
  127.             }, context_instance=RequestContext(request))
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/shortcuts/__init__.py" in render_to_response
  20.     return HttpResponse(loader.render_to_string(*args, **kwargs), **httpresponse_kwargs)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/loader.py" in render_to_string
  188.         return t.render(context_instance)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/base.py" in render
  123.             return self._render(context)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/base.py" in _render
  117.         return self.nodelist.render(context)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/base.py" in render
  744.                 bits.append(self.render_node(node, context))
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/debug.py" in render_node
  73.             result = node.render(context)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/loader_tags.py" in render
  127.         return compiled_parent._render(context)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/base.py" in _render
  117.         return self.nodelist.render(context)
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/base.py" in render
  744.                 bits.append(self.render_node(node, context))
File "/srv/http/elmo.stasmade.com/elmo/vendor/src/django/django/template/debug.py" in render_node
  73.             result = node.render(context)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/templatetags/compress.py" in render
  91.         rendered_output = compressor.output(self.mode, forced=forced)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/css.py" in output
  55.         return super(CssCompressor, self).output(*args, **kwargs)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/base.py" in output
  214.         verbatim_content, rendered_content = self.filtered_input(mode, forced)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/base.py" in filtered_input
  173.         for mode, hunk in self.hunks(mode, forced):
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/base.py" in hunks
  131.         for kind, value, basename, elem in self.split_contents():
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/css.py" in split_contents
  20.         for elem in self.parser.css_elems():
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/utils/decorators.py" in __get__
  39.             value = obj.__dict__[self.__name__] = self.__get(obj)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/base.py" in parser
  105.         return get_class(settings.COMPRESS_PARSER)(self.content)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/parser/__init__.py" in __init__
  19.         self._setup(content)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/parser/__init__.py" in _setup
  28.                 self._wrapped = parser(content)
File "/srv/http/elmo.stasmade.com/elmo/vendor-local/src/django_compressor/compressor/parser/lxml.py" in __init__
  20.             raise ImproperlyConfigured("Error while importing lxml: %s" % err)

Exception Type: TemplateSyntaxError at /teams/nb-NO
Exception Value: Caught ImproperlyConfigured while rendering: Error while importing lxml: No module named BeautifulSoup
More likely, django_compressor relies on lxml and uses BeautifulSoup as a fallback. In your case it appears that neither are available. The bug is to put lxml into requirements/compiled.txt I guess. Strange that missed that in the django_compressor requirements.
Actually, I think it's having lxml and not having BeautifulSoap. I don't seem to have lxml and it works just fine. Anyway, that seems to be a problem not related to this bug, should we file a new bug to clarify that?
I don't think that tree_to_application should just go for the first two chars, but for the real app code as the start of the tree code. Also, probably ordered by length, so one could do fennec_native vs fennec, say.

The commented code about rowspan is to support the real grouping of the table?

I'm a bit concerned about manipulating the db Run objects, that feels scary and not totally future proof.
Comment on attachment 574482 [details] [diff] [review]
New team home page (with Jasons HTML)

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

r- for the comments below.

::: apps/shipping/views/__init__.py
@@ +112,5 @@
> +    def tree_to_application(tree):
> +        return _trees_to_apps[tree_prefix(tree)]
> +
> +    def tree_to_appversion(tree):
> +        return _trees_to_appversions.get(tree)

The mapping here is too brittle. We should rather go for the real app code, and if that prefixes the tree code, the two belong to each other. I'd even go as far as sorting the tree code by length, so you can do fennec and fennec_xul, for example.

@@ +124,5 @@
> +        #    application.rowspan += 1
> +        #else:
> +        #    application.rowspan = 1
> +        #    _previous_application = application
> +        #run.application = application

is this the start to group the table? I'd rather use it or remove it, adding commented out code feels funny.

@@ +127,5 @@
> +        #    _previous_application = application
> +        #run.application = application
> +        run.changed_ratio = run.completion
> +        run.unchanged_ratio = 100 * run.unchanged / run.total
> +        run.missing_ratio = 100 * run.allmissing / run.total

Here and below, you're changing the db object, I'd rather not do that. It feels dirty, and it's at least not future proof if we were to add more data.

Go for dicts rather?

Also below.
Attachment #574482 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] from comment #21)
> Comment on attachment 574482 [details] [diff] [review] [diff] [details] [review]
> New team home page (with Jasons HTML)
> 
> Review of attachment 574482 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r- for the comments below.
> 
> ::: apps/shipping/views/__init__.py
> @@ +112,5 @@
> > +    def tree_to_application(tree):
> > +        return _trees_to_apps[tree_prefix(tree)]
> > +
> > +    def tree_to_appversion(tree):
> > +        return _trees_to_appversions.get(tree)
> 
> The mapping here is too brittle. We should rather go for the real app code,
> and if that prefixes the tree code, the two belong to each other. I'd even
> go as far as sorting the tree code by length, so you can do fennec and
> fennec_xul, for example.
>

I read that a couple of times but I actually don't understand your point. Can you please elaborate.
* Fixes the sort order so that it depends on the real application codes. Also, does it in sort order by key length.

* Instead of returning instances of the Run model, it returns instances of a dict subclass that just adds basic dot notation. This makes it impossible to accidentally fire methods on the Run instances such as .save()

* All other commented out crap deleted. Also ran pep8 check on the relevant code.
Attachment #574482 - Attachment is obsolete: true
Attachment #580087 - Flags: review?(l10n)
Comment on attachment 580087 [details] [diff] [review]
Fixes from review

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

r=me.

I had one nit, could you stop calling into l10nstats.views.teamsnippet? It's adding more queries to the database still, and we're not using it at all. I'd put ripping out the old code into a follow-up, though.

Running locally was kinda slow, I guess we need to revisit some queries and make sure they're actually good to make that way. Notably, the pushes and the Run_Revisions queries in annotated_pushes look like good candidates. Maybe move the "ignore branch pushes" in the first to python code, and I wonder if there's a larger dataset in the Run_Revisions to take as an intermediate as well.
Attachment #580087 - Flags: review?(l10n) → review+
Landed https://github.com/mozilla/elmo/commit/ffd5d74b7ef5cf8bb1aea797bfb7cc16e2ace28c

I don't know what you mean by "stop calling into l10nstats.views.teamsnippet". Would you mind elaborating on that a bit? It's been a while since I looked at the code. 

Also, regarding the optimization, I'm sure we can file more followup bugs but here's one: https://bugzilla.mozilla.org/show_bug.cgi?id=713878
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
:stas after a lot of code *reading* I think I've understood what's going on. 

django_compressor parses the template code with lxml if it's available and otherwise the builtin HTMLParser. In my virtualenv, where I don't have lxml installed (or BeautifulSoup) it falls back on lxml. 

When you run it with wsgi, you're probably not using your virtualenv and you probably have a bad lxml installed. lxml.html has a builtin hack that sets up its own fallback parser which is BeautifulSoup. That's because BeautifulSoup has more tasty hacks to deal with bad Unicode and stuff. Why you can't import lxml without BeautifulSoup simply smells like a bug or a bad installation of lxml. Try this outside your virtualenv:

 >>> from lxml.html import soupparser

Another vague theory is some other python importing bug. Strangely, the wrapper in django_compressor that does all of these things with the lxml.html module is named "lxml.py". Perhaps that's an explanation. Doubt it but could be.
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: