951348 - Cannot remove an app from a collection from rocketfuel

Status: VERIFIED FIXED

(Marketplace Graveyard :: Admin Tools, defect, P2)

Description

[Christopher Van Wiemeersch [:cvan]]

STR:
1. Go to a collection.
2. Click the "X" for an app to delete it.
3. Notice JS error:
Uncaught TypeError: Cannot read property 'id' of undefined
http://f.cl.ly/items/0Y020e3O3L3T2p0D2x2w/Screen%20Shot%202013-12-17%20at%2012.08.08%20PM.png

NOTE: Reproduce in prod, stage, and -dev:
https://marketplace.firefox.com/curation/collection/games-col-featured
https://marketplace.allizom.org/curation/collection/featured-apps
https://marketplace-dev.allizom.org/curation/collection/wowowowo

-

This fixes the problem somewhat such that the model cache look up works, but then I see a "[Object][object] was deleted" notification:

diff --git a/src/media/js/views/collection.js b/src/media/js/views/collection.js
index 22d8508..921bee8 100644
--- a/src/media/js/views/collection.js
+++ b/src/media/js/views/collection.js
@@ -185,7 +185,7 @@ define('views/collection',
         $app.hide();
         var app_id = $app.data('id');
         var collection = get_collection();
-        var app = app_model.lookup(app_id + '', 'id');
+        var app = app_model.lookup(app_id, 'id');
 
         requests.post(
             urls.api.url('remove_app', [collection.id]),
@@ -212,7 +212,7 @@ define('views/collection',
         var app = $this.data('id');
         $this.parent().remove();
         var collection = get_collection();
-        var app_data = app_model.lookup(app + '', 'id');
+        var app_data = app_model.lookup(app, 'id');
         var $app = $(nunjucks.env.render(
             'helpers/app.html',
             {'this': app_data, 'allow_delete': true, 'allow_reorder': true, 'tag': 'li'})

I clearly don't know what I'm doing.

Comment 1

[Matt Basta [:basta]]

The [Object object] is because the API now returns locale objects; we need to run that through `utils.translate()`.

It looks to me like there's a deeper issue. I'll look at the API, but what it seems like is the API is now returning a number for the ID rather than a string containing a number. If that's the case, then the fix in comment #0 should be fine to fix the lookup (I think). I'll investigate further.

Comment 2

[Chuck Harmston [:chuck]]

This one is on the API.

I made the mistake of attempting to JSON-decode data in a branch that got run regardless of what the Content-Type header was set to, so it's flailing on Rocketfuel's application/x-www-form-urlencoded.

PR coming shortly.

Comment 3

[Chuck Harmston [:chuck]]

Sorry, I'm extra dumb today. I conflated this with bug 951353. Carry on...

Comment 4

[Matt Basta [:basta]]

Upon further investigation, the fix in comment #0 (despite being completely non-obvious) is the correct fix. IIRC, we might even be able to remove the second parameter ('id'), since that's the primary key for the collections model cache, anyway. I could be wrong. We may never know.

Comment 5

[Matt Basta [:basta]]

https://github.com/mozilla/rocketfuel/commit/c0382dd174893a9d0de926ad07805a0513164a35

Comment 6

[krupa raj[:krupa]]

verified fixed @ https://marketplace-dev.allizom.org/curation/collection/new-collection