Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: chrome/content/ui/sendReport.js

Issue 29333150: Issue 3461 - Fixed: Issue Reporter assumes that RequestEntry.filter is a Filter instance, not a str… (Closed)
Patch Set: Created Jan. 4, 2016, 5:46 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/content/ui/sendReport.js
===================================================================
--- a/chrome/content/ui/sendReport.js
+++ b/chrome/content/ui/sendReport.js
@@ -820,18 +820,24 @@ var subscriptionUpdateDataSource =
return;
FilterNotifier.removeListener(listener);
E("updateInProgress").hidden = "true";
let filtersRemoved = false;
let requests = requestsDataSource.origRequests;
for (let i = 0; i < requests.length; i++)
Sebastian Noack 2016/01/04 18:14:52 Nit: While on it, how about using a for-of loop?
Wladimir Palant 2016/01/04 19:23:44 There is too much to be fixed in this code already
Sebastian Noack 2016/01/05 09:20:52 Fair enough.
- if (requests[i].filter && !requests[i].filter.subscriptions.filter(s => !s.disabled).length)
+ {
+ if (!requests[i].filter)
+ continue;
+
+ let filter = Filter.fromText(requests[i].filter);
+ if (!filter.subscriptions.filter(s => !s.disabled).length)
Sebastian Noack 2016/01/05 09:20:52 How about |filter.subscriptions.some(s => !s.disab
Wladimir Palant 2016/01/05 09:50:08 This functionality isn't exactly easy to test. But
filtersRemoved = true;
+ }
if (filtersRemoved)
{
// Force the user to reload the page
E("updateFixedIssue").hidden = false;
document.documentElement.canAdvance = true;
let nextButton = document.documentElement.getButton("next");
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld