396741 - Implement new categories list (CAT-1)

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Basil Hashem [:baz]]

As specified in the AMO v3.2 PRD (Update:RequirementsV32) and the mockups (http://wiki.mozilla.org/Update:Remora_UI_Review/Mockups/Home_Page/2007-09-12_revisions/)

Comment 1

[Basil Hashem [:baz]]

Search will have subcategories as follows:
Bookmarks
Business
Dictionaries & Encyclopedias
General
Kids
Meta-search
Music
News & Blogs
Photos & Images
Shopping & E-Commerce
Social & People
Sports
Travel
Video

Thankfully, we can make these changes via the admin control panel on AMO so it should not require extra coding. (The current version of AMO disables the category list view for search engines).

Comment 2

[Basil Hashem [:baz]]

All search categories have been created using the Category Manager (https://addons.mozilla.org/en-US/firefox/admin/tags) on production AMO. Need to ensure that the search engines categories will appear on the new search categories page per the design.

Adding dependency on bug 396737 & reassigning to Morgamic

Comment 3

[Fred Wenzel [:wenzel]]

Have we notified localizers yet to look at the translations? I will do the German ones now, but until last week's meeting I didn't realize I had to redo some of the category names and descriptions; the other localizers probably don't know either.

Comment 4

[Basil Hashem [:baz]]

From the last AMO meeting (12/11/07), Mike was going to assemble the list of changes and Wil was going to draft the email so that we don't keep hitting them up with additional translation tasks.

Comment 5

[Michael Morgan [:morgamic]]

Not sure why this assigned to me, but we can talk about it tomorrow.

Comment 6

[Basil Hashem [:baz]]

The search results page and the category list page (http://mozilla.focalcurve.com/categorylist.html ) share a similar element, the "drop down" list for the number of add-ons to show.

The current mocks show only 5, 10 and 20. We need to expand this list to include: 5,10,20,50,All.

Thanks.

Comment 7

[Basil Hashem [:baz]]

To prevent database blow up, we'll only do 5,10,20,50,100 and not ALL.

Comment 8

[Fred Wenzel [:wenzel]]

Please make sure the "special case" top-level categories are renamed at the time of the relase as follows:

- "Language Tools" becomes "Language Support"
- "Interface Customizations" becomes "Themes & Appearance"

("Search Tools" stay this way).

Comment 9

[Michael Morgan [:morgamic]]

We need to fix sorts and stuff, so this depends on bug 412645.

Comment 10

[Ngamer01]

One other thing.  If the Sandboxed add-ons will now be visible to guests, what about all the Adult add-ons?  If you all keep all adult add-ons on site, you're going to need to enable some kind of age checking verification and other whatnot to make sure kids don't see these sleazy add-ons.

Comment 11

[Basil Hashem [:baz]]

OK, made the requested changes on production AMO in order to help prep for localization. "Interface Customizations" is now "Themes & Appearance" and "Language Tools" is now "Language Support".

Comment 12

[Michael Morgan [:morgamic]]

Will fix this once we land Chris' cat sorting patch.

Comment 13

[Michael Morgan [:morgamic]]

Baz, you should be able to reorder the categories with the proper weights now via admin panel -- there isn't anything else to do with this bug.

Comment 14

[Basil Hashem [:baz]]

This is hot - Chris or Mike, just to confirm - can you explain briefly how the weighting works? 0 means natural alphabetic? yes?

When I chose a value of 10 for say Other, it dropped to the bottom. So, take all the zero's sort alpha and then the next highest weighting and sort alpha? Is that right?

Comment 15

[Basil Hashem [:baz]]

I'm reopening for a small issue. I went into the "Miscelleaneous" category (which is marked as global for all applications) and added a weighting of 10 and it changed the menu item to say "Array"... see https://preview.addons.mozilla.org/en-US/thunderbird/. Note that there are TWO "Miscellaneous" categories (both marked as global for all apps) and I changed both to be weight=10. Thanks.

Comment 16

[Michael Morgan [:morgamic]]

Chris, take a look at this tomorrow.

Comment 17

[Chris Pollett]

Here is the code that currently pulls the categories for that drop down:

        return $this->controller->Tag->findAll( 
            array(
                'application_id' => $app,
                'addontype_id'   => $type
            ),
            null,
            'Tag.weight, Translation.name'
        );

It is in app controllers components elements amo.php.
In the category weight bug my contribution was to add Tag.weight. Currently,
the first argument to findAll is the default value which is the empty string/NULL. This first argument controls the WHERE clause in the DB query and so says there isn't a where clause. The code should probably be changed to:
       return $this->controller->Tag->findAll(
            "application_id = '$app'",
            array(
                'application_id' => $app,
                'addontype_id'   => $type
            ),
            null,
            'Tag.weight, Translation.name'
        );
so that it restricts the categories by application, in the case of Miscellaneous, this will mean rather than get an array back we would get just one item. The problem is I am having a little bit of difficulty recreating this bug on my home system to test this.

Comment 18

[Chris Pollett]

My above solution, doesn't work and does not seem to be relevant to the problem.

Bug 396741 is caused whenever you try to update a category for which their are multiple copies of that category name with the same selected application and addon type. For example, multiple categories named Miscellaneous, with application "All" and addon type extension. Even if you click update without changing anything, the name and description fields will be set to the value Array. I am not sure why we are allowing multiple categories with same name like this, but call such a  condition (*).


I used a dump of the production database and tried to track down the problem. 
I am getting stuck, so I am hoping writing this will either unstick me, or at least will invite suggestions of why I am stuck. As far as I can tell so far, something weird is happening in the beforeSave method of AppModel. When a category is updated, this method is called several times to update the translations:

(1) It is called a first time and does an update like:
UPDATE translations SET localized_string = 'Array', modified = NOW() WHERE id = 1089 AND locale = 'en-US';
the string Array come from $data[$this->name][$tr_field] which in this case is an array of all translations for every locale. (1) Although it is pointless this update happens whenever a category is updated, regardless of whether (*) holds or not.

(2) beforeSave is then called to update the localized strings for each locale. For instance, you might have something like:
UPDATE translations SET localized_string = 'Miscellaneous2', modified = NOW() WHERE id = 1089 AND locale = 'en-US';
as well as similar updates for german, french ,etc.
The updates that are attempted when (*) holds versus when it doesn't are essentially the same; so this does not seem to be the problem. 
However, the update that would replace the bogus update (1) for en-US does not occur, in situation (*). Moreover, if you wait for the second attempt at an en-US update then write your own mysql_query to do some update of the translation id in question, say 1089, it takes forever when condition (*) holds and it doesn't seem to actually do the update. However, if you run this same code but where the category being updated doesn't have condition (*), it is almost instantaneous, and will update the id. mysql_stat seems to indicate in both cases the connection is alive.

Comment 19

[Chris Pollett]

I slightly misspoke in the above: When a
category is updated, this method is called several times to update the
translations:

should have read:

When a category is updated, this method performs several updates to the
translations table:

Comment 20

[Chris Pollett]

Attachment: patch to solve problem

This patch adds two lines: (1) a commit statement in app_model which forces the translations to be committed. This solves the problem of making the second update described in comment 18 above to actually take effect in the database. (2) The second line change in the _editTag method of admin_controller forces the update of weight to occur if you are in condition (*) described in comment 18.

The patch seems to ameliorate the symptoms, but I am still not exactly sure what is causing the problem.

Comment 21

[Chris Pollett]

Okay, I applied, the above patch

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on both preview and production (sorry for not verifying sooner!)