677588 - Webui should use middleware to get current and featured product versions

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[Brandon Savage [:brandon]]

In Socorro 2.2 we reworked the webui to use product_info instead of productdims. However, this change was not moved into the middleware and should have been.

Comment 1

[Brandon Savage [:brandon]]

This should be done today but will require a touch more testing than I can get in before freeze. This will just barely miss 2.3 so pushing off for next week's milestone.

Comment 2

[Brandon Savage [:brandon]]

A patch has been added to my github repo. It's pretty much ready to go, just needs a little polish to ensure we don't add a security vuln in the middleware.

Comment 3

[Brandon Savage [:brandon]]

Awaiting review from :adrian and :lonnen.

Comment 4

[[DEACTIVATED] Adrian Gaudebert]

A few comments on [https://github.com/mozilla/socorro/pull/108/files]:

- The Python part looks good, it will have to be rewritten as part of the middleware reorganizing soon but for now this is the good way of doing it. There is a problem with parameters that seem to have no impact on ordering. 

> def createOrderBy(user_input):
To go really PEP8, function names in the new parts should be lower cased. 

- Now about the PHP part, in file branch.php: first I see some inconsistencies in code style, mainly some "if(" without a white space before the parenthesis. Same for some foreach. Convention is one white space after a control structure keyword and no space after a function name. (Read [http://pear.php.net/manual/en/standards.php] referenced by [https://wiki.mozilla.org/Socorro/SocorroUI/Coding_Standards])

Also operators in big conditions should be at the beginning of a line and not at the end. 

I don't know if it really matters to strictly follow the conventions here. We tend to consider we will "soon" get rid of PHP and consequently do not pay much attention to the code quality. Maybe that's a mistake and we should be as strict here as we are regarding Python and PEP8. 

>+        $cache_key = '/* soc.web branch.prodversions */';
>
>         if ($delete_cache) {
>-            $this->cache->delete($this->queryHashKey($sql));
>+            $this->cache->delete($this->queryHashKey($cache_key));
>         }
Is that needed? I didn't find any reference to that cache key anywhere else in the file. What is that code doing?

Finally, I guess SQL queries that are still in that file are ones we cannot rewrite using the midlleware yet?

That's a r- for now. If you fix that bug and consider my questions are stupid, then please mentally change it to a r+. Also I'd like :lonnen to test it when it's fixed if possible.

Comment 5

[Brandon Savage [:brandon]]

I've made some additional changes largely in line with your comments here. I fixed the bugs in the middleware and removed the useless caching components. I didn't worry much about the PHP formatting, since we are ditching PHP at some point. And yes, there are some SQL queries we cannot rewrite yet using the middleware, which remain in the original.

Comment 6

[[DEACTIVATED] Adrian Gaudebert]

I tested it by calling the middleware directly and hitting a few pages of the webapp, it seems to work fine. The code looks good too, I didn't see anything suspicious. 

Running check.py on currentVersions.py fires a few errors though, you might as well fix those if possible.

socorro/services/currentVersions.py:4: 'db' imported but unused
socorro/services/currentVersions.py:7: 'dtutil' imported but unused
socorro/services/currentVersions.py:13: 'defaultdict' imported but unused
socorro/services/currentVersions.py:84: local variable 'defaultdict_factory_with_list' is assigned to but never used
socorro/services/currentVersions.py:87:80: E501 line too long (106 characters)
socorro/services/currentVersions.py:88:25: E201 whitespace after '{'

After those are fixed, it's a r+ for me. Feel free to merge it in.

Comment 7

[[github robot]]

Commits pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/ddc819ceab4114719c209e5891b28054ffb9a96d
Bug 677588 - Making use of the middleware for providing version data.

https://github.com/mozilla/socorro/commit/ea740cbfed4949db82a083f9d19834720da93bcf
Merge pull request #108 from brandonsavage/bug677588

Bug 677588 - Making use of the middleware for providing version data. r+ from Adrian.

Comment 8

[[github robot]]

Commit pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/c97de4297652edd5ced3df657b454ceaa84b34c8
Merge pull request #119 from brandonsavage/bug677588

Bug 698338 - Fixing incorrect reference to a property that caused Kohanna