63782990-1748-u This could be a little more simple/pythonic ```python product_name = NEW_APP_NAME if vconfig.is_feature_enabled('presence_builder_rebranding', self.pid) else APP_NAME context.update({ "product_name": context.get('wl_product_name', product_name) }) ```
63784194-1748-u It'd be nice to one-line this: ```python context.update({ 'product_name': context.get('wl_product_name', NEW_APP_NAME if vconfig.is_feature_enabled('presence_builder_rebranding', self.pid) else APP_NAME) }) ```
63784250-1748-u Breaking maybe right before the `if`. Whatever makes sense when you've got it in PyCharm
63784331-1748-u Well I commented the same thing as Jesse. Gotta reload these pages every once in a while.
63604137-1747-u Maybe `enable_listing_builder_rebrand`?
63604244-1747-u Maybe.
63604419-1747-u Minor complaint but you shouldn't really be checking against the builtin Bool objects. Something like `{% if not enable_listing_builder_rebrand %}`
63604684-1747-u Why are your imports so afraid of each other? They can be closer together.
63605191-1747-u You have the same 2 patch decorators on 3 different tests. Typically in this case it's cleaner to set up your mocks in the `setup` method. I'll show you how to do this.
63605815-1747-u Nice, I forget to do that pretty often
63605869-1747-u Thank PyCharm's auto importing ?
62937629-1742-u Does this test actually assert or check anything?
62937833-1742-u I guess it didn't before, either. We can probably do something like this: ```python field = GenericJsonField() field.data = '[]' try: validate_json(field, []) except validators.ValidationError: self.fail() ``` Whatcha think @jrolheiser-va ?
62937982-1742-u I kind of like the `with self.assertRaises` pattern a bit better, just because you actually call the function so it makes finding usages of it easier (I think, that might not be true haha). Like this: ```python with self.assertRaises(validators.ValidationError): validate_json(field, 'url') ```
62938565-1742-u I'm not sure if I understand what you're saying...
62940157-1742-u Okay, I see. I think the difference between our two approaches is that the self.fail() is explicit whereas yours is implicit. I think explicit is better in this case.
63040952-1742-nu Personally I find this sort of structure a little confusing, is it `not (values and required_fields)` or `(not values) and required_fields`? I'm sure it works properly, but maybe include the parens for clarity?
63041774-1742-nu https://docs.python.org/2/reference/expressions.html#operator-precedence Interesting, the `and` will get calculated first which isn't how I thought worked. Whoops.
62267796-1738-u Could probably condense these three if statements
61961593-1733-u Awe so long popularity circles. Mind removing `popularity-circle` `.popularity-circle.popular` and `.popularity-circle.unpopular` from `_profile-sync.scss`
62066961-1733-nu Could maybe change this to ``` self.showField = self.status in FIELDS_STATUSES_TO_SHOW ``` for brevity's sake
62069524-1733-nu Yeah, I guess `in` doesn't work like that. I'm cool with your alternative.
61960790-1730-u Should set a warning flash or an error flash for this.
61943853-1726-u Seems like a bunch of duplication. Can't extract it at all?
61798454-1722-u I'm guessing it's right too. The only way I know to get what it actually is, is to look at a source that has a listing on the source, and see where that url takes you. Might be hard to find one though.
61798555-1722-u Should this be `opendi`?
61799184-1722-u I believe, for Canada at least, it's called Opendi. https://www.opendi.ca/ Not sure about the U.S. Stadtbranchenbuch appears to be the German version.
61510090-1715-u Surely this, or something like this, is in _colors.scss?
61510378-1715-u @bjohnson-va this is the css file. The ```_profile_sync.scss``` is using the scss variable for the colour
61510777-1715-u does it have to be ```eaeaea```? We have ```$SECONDARY_BACKGROUND_COLOR = #efefef``` which is pretty damn close
61511277-1715-u I'd prefer standardization over mock-following. Your call.
61511327-1715-u Dammit. Why do we commit our CSS files :C
61581964-1714-nu this is significantly easier to read ?
61346453-1710-u Good call. Grab the one from the AccountGroup.
61333312-1709-nu Does it need a type or popularity?
61168438-1706-u @rwiebe-va I think this overlaps with your work in https://github.com/vendasta/MS/pull/1705.
61168852-1706-u Ah yes it does... We can just do them here I suppose.
61169357-1706-u No worries, couple things for this: - totalListingSyncProSources appends a space on to the end, so this will have two spaces after it. - Could you add "additional" after the span to make it read a little better while it loads. - Also could remove the "+" being appended. i.e. https://github.com/vendasta/MS/pull/1705/files
61168235-1705-nu Clever.
61104603-1700-nu So "technically" the `microsite` is not guaranteed to be present (hence the `hasattr` check at the top of the method) however it looks like the only handler that uses this handler (besides `AuthenticatedEditSiteView` which requires msid) is `GetOneMonthlyBillingReportForPartner` so you are pretty safe. Having said that you might just want to add a `if not microsite: return False` at the beginning of this method...maybe?
61104950-1700-nu This is super minor but I would replace `market_id` here with `getattr(self.microsite, 'market_id', None)` and remove that stuff at the top of the method since we only need it here and, depending on the config, the code might not even get here.
60961274-1697-u could be ```.free-listing-sync h1``` since we have nothing underneath
60961367-1697-nu Why did we add width here?
60961401-1697-nu and here?
60917718-1692-u We should refactor the code in `sync.py` to use this `SocialSync(self.microsite.msid).get_number_synced_profiles()` method.
60917794-1692-u Can delete.
60926312-1692-u Looks like this is based off an older version, may want to rebase. But this should just be empty now instead of "--".
60766099-1690-nu we don't have to check for empty string either?
60755184-1689-u do we often leave in log statements?
60755659-1689-nu Could move this logic to a function and add some jasmine tests perhaps.
60669014-1688-u Why is this necessary? Your jinja condition was already doing this logic.
60669369-1688-u When that happens, you often just need to be more specific with your sass. Eg: ``` .outer_div { .inner_div { .additional_wrapper { .sync-pro-header-right { width: 290px; } } } } ```
60736152-1688-nu If it's in sass that's the case, but inline styles trump everything that isn't marked with ```!important```
60754888-1688-u Crazy idea. We could use the [jinja set keyword](http://stackoverflow.com/questions/3727045/set-variable-in-jinja) to avoid duplicating this in multiple spots ``` { % set show_listing_sync_pro_sections = listing_sync_pro_enabled or listing_sync_pro_active_for_account % } ``` ... and then use `show_listing_sync_pro_sections` instead of these boolean pairs.
60667112-1687-nu AKA: normal http://htmldog.com/references/css/properties/font-weight/
60667257-1687-nu This 620 was probably supposed to go in an scss file, right?
60669637-1687-nu To have the value in SASS, you would, basically, need to add an ID to the outer div of the modal which gets added by jQuery after the page loads. Without having experience with doing that, it might not be worth bothering. This is fine.
60746981-1687-u You don't need to use a computed property here since, after page load, the `salesPersonImage` isn't going to change. The computed property is for when you want this property to change based on another property changing within the page (usually based on user interaction). ```javascript self.salesPersonImage = salespersonData.salesPersonImage || '/static/images/default-salesperson.svg'; ```
60747127-1687-nu I moved a bunch of styles into `_profile-sync.scss` for PRD-89 so you wouldn't have known. This file should just be for common admin styles.
60749749-1687-nu The dialog binding accepts a width attribute. You should be specifying it here instead of the scss. It is much simpler than trying to style the constructed modal. It's pretty much the same as if you were using a jquery UI dialog directly where you pass this as options on instantiation (which is what the binding is doing).
60658765-1684-u See my comments: https://github.com/vendasta/MS/pull/1683
60658878-1684-u You mean "Free Listing Sync" right?
60659192-1684-nu Lol man it sucks doing this in knockout.
60633774-1683-nu Is this getting formatted correctly? There is a jinja filter for display our datetime objects in a human readable format.
60655089-1683-nu Instead of this I think you should be using a filter: https://github.com/vendasta/MS/blob/develop/src/app/views/jinja_ext/util.py#L49 Also if you do it the way you've shown above you should be using a constant to make sure we have consistent date(time) formatting.
60626265-1682-u yeah @rwiebe-va that's wrong...
60491475-1674-u Since you're here, I would just rip out the inline stuff as well
60493592-1674-u wouldn't both of those be true, anyway?
60489793-1673-nu whoa, that's a mouthful
60492817-1673-nu Of course you'd say that lol
60468974-1670-nu Not really important, but should we pare this down to just `[GET]`?
60476145-1670-u I don't think the `send_message()` is needed here. When `open_dialog()` is triggered on the click of the `Activate` button it checks if there is no sales person linked to the account. If that is the case it triggers the ajax call to set hotness in ST. If there is a sales person linked to the account nothing is triggered - as the trigger will occur when they click the `Send` button in the modal. Maybe this was a poor way of doing it, but the issue was that we wanted the hotness to be triggered on the clicking of the activate button on the banner if there is no salesperson. But if there is then it was to be triggered on the actual sending of the message.
60314304-1668-nu Is it ever possible to get a failure?
60314824-1668-u Doesn't need to be declared outside the conditional and then reassigned. It's only used the one time
60315071-1668-u we should be leaving a line in between each rule, including nested ones
60315341-1668-u Should be indented under the jinja statement
60315494-1668-nu LSP should probably be title cased since it's a name
60322745-1668-u Depends who you ask. But, agreed.
60322846-1668-u "sale**s**"
60323021-1668-u I thought that was the expectation of our scss linter?
60323543-1668-u "sale**s**" representative
60323812-1668-nu May want to check if this account group was found for the account group id before `user_interested_in_listing_sync_pro` and if it doesn't you could probably return `{ 'success': False }`
60325403-1668-nu I think that should be the responsibility of the API endpoint in salestool. Let's keep this AJAX endpoint lightweight.
60415167-1668-nu Just a reminder we have to setup the pubsub topics on MS/ST for each environment when they are deployed.
60415258-1668-u Super minor, but technically this bracket should not be indented.
60415581-1668-nu In the future I would just return the `sales_person_details` dict from this method and then if the view (or whoever wants to make use of this method) chooses to serialize it to json they can do that on their own. As it is right now if, for example, they wanted to add an extra key/value they would have to deserialize, add key/value, and then serialize it again which is obviously not ideal.
60416233-1668-nu Only problem is this code will fail on the next line because `partner_id` and `market_id` don't exist for `None` object in which case we should probably return a 404 or something. I don't think it's a big deal though because it's just an internal ajax endpoint that will still fail if the `account_group` doesn't exist anyway (in which case how are they even hitting this endpoint?).
60416322-1668-nu Given that it's an internal ajax endpoint if something goes wrong it'll just raise an exception which I think is probably okay unless we find a specific error scenario we want to handle.
60416676-1668-nu I don't think we should block this from getting merged but we should update ST (if it hasn't been done already) to support `MS`. Otherwise this is going to get super difficult to maintain because in ST it looks like `VBC` is hitting their api.
60416813-1668-nu Same here.
60416834-1668-nu Same here.
60416852-1668-nu Same here.
60416998-1668-nu In the future JS should be written with methods/variable using `camelCase` instead of `snake_case`.
60417175-1668-nu Lol empowertising.
60417477-1668-u Just fyi, when you are writing code that you know will be worked on in a separate issue it never hurts to include something like: `TODO: PRD-73: update sync numbers`.
60301727-1667-nu Won't this mean MS won't be updated but the change will remain on AA so the two will be out of sync?
60302005-1667-nu Where/how is this set?
60294188-1666-nu I assume you'll change this `45` to something real in PRD-73?
60294490-1666-nu Will it always be 45 on prod? I know Pirates are adding Canada sometime this sprint, which will have a different number of sources, so we can calculate the total when we do that. Edit: Brad is right, PRD-73 looks like the story for that.
60297474-1666-nu Definitely should not be a constant (i.e. 45) we can just pull from the list of directories we have. Can address in PRD-73.
60297547-1666-nu The number of directories can and will change without our involvement so it needs to be dynamic.
60239978-1662-u Instead of requiring two separate variables I would just use `self.wl_listing_sync_pro_enabled` and `listing_sync_pro_feature_enabled` to determine if `listing_sync_pro_enabled`. I'm thinking... ```python 'listing_sync_pro_enabled': self.wl_listing_sync_pro_enabled and listing_sync_pro_feature_enabled ``` Just makes the logic in the template simpler and I don't think there is a need to send them separately.
60240041-1662-u See my note above, would simplify these logic statements.
60240098-1662-u Minor, please leave a space between your variable and `%}`.
60249881-1659-nu I can't really tell, but you might benefit from caching the result of `get_social_sync_profiles` I think it's used multiple times within this class. I can't really tell from the PR if it would be used more than once during a single instance of the class though.
59642523-1649-u Well *this* file shouldn't be here :D
59642634-1649-nu You're re-using the `message` variable here. Maybe rename it to avoid confusion.
59642944-1649-nu I generally recommend opening a separate, direct-to-master, pull request for upgrading libraries. This helps avoid conflicts with other people who want to upgrade the library to a still-newer version. It would also make **this** PR a lot easier to navigate.
59643095-1649-nu Is there not an 'ms' API user for Sales Tool? Probably shouldn't be pretending to be another project here.
59643276-1649-u Probably worthwhile to add a "spinner" icon while this message is being sent.
59643447-1649-u Can use `$PRIMARY_BACKGROUND_COLOR` here. Check out `static/sass/partials/_vff_colors.scss`
59643471-1649-u `$PRIMARY_BORDER_COLOR`
59754235-1649-u This method should probably take an account group id and use it here, (and below) right?
59763077-1649-nu `dealing with triggering hotness when user shows interest` Sounds like friday night.
59763755-1649-u This should be a domain method.
59764369-1649-u I would recommend putting this in a constant at the top of the file so it is more visible and clearly defined. ```python LSP_PRICE = 24.99 ```
59764555-1649-u Will the `sales_person` always have a key that is the `agid`? If this is just checking that condition it will throw a `KeyError` if it does not exist. You might want to use `sales_person.get(self.microsite.agid)` instead which only returns the value if that key exists and `None` by default.
59765493-1649-nu Minor, but I think this would be a bit more readable if you grabbed the account group off the sales person first. Part of this is just preferred style but something like: ```python account_group = sales_person.get(self.microsite.agid) sales_person_details = { 'salesPerson': '', 'agid': self.microsite.agid } if account_group: sales_person_details.update({ 'salesPerson': account_group['firstName'], 'email': account_group['email'] # etc. }) return json.dumps(sales_person_details) ```
59765702-1649-u I believe you can just use `vapi.NoAuthAjaxHandler` instead of needing the `BaseAjaxHandler`.
59766066-1649-u Minor, but I'm pretty sure by default the name is set to the variable name. I think the only reason you would want to specify `name` is if the arg name you were accepting via the api is different than the variable name you want to use in the code here.
59766311-1649-u Generally we don't want to catch all the exceptions, only certain ones (if any) that you expect the code might throw and want to handle in a specific way.
59766558-1649-nu I agree, but using Brent's [Pretty Pull Requests](https://chrome.google.com/webstore/detail/pretty-pull-requests-gith/ljnjpkadhhcdniohpfilddnhahoigdec?hl=en) extension you can collapse files/directories you don't want to look at which makes it more bearable.
59766704-1649-u I wouldn't worry too much if you are in a rush but `_api` is essentially already used for these ajax apis so if possible (or in the future) I would use that instead: https://github.com/vendasta/MS/pull/1643/files#diff-08e3d0d956fd7d5dbe9ef8f1b85bf8c1R211
59766859-1649-nu Some unnecessary indenting going on here.
59766961-1649-nu Minor but in JS we should be using `camelCase`. It looks like you are for `salesPerson` and some of the other fields so please keep that going.
59767610-1649-u For colors you should always try to use the existing scss variables if possible. They will be in the same file as the `$PRIMARY_BACKGROUND_COLOR` variable. I don't know off hand if this specific color is described there but there definitely is one for `#fff` or `white` that you are using below.
59767695-1649-u Any reason for removing that `vurl`?
59767953-1649-u The indenting here is a bit off but please leave because I adjusted it in PRD-89 so if you change this it will just create merge conflicts. In the future please indent anything inside a jinja `{% if ... %}` or knockout `` block similar to how you would in python.
59768223-1649-u You need your JS to also be behind a `{% if is_listing_sync_pro_enabled %}` block otherwise, if it is not enabled, currently the JS will still try to execute but the html will be missing (not rendered) which will likely cause errors to occur. You can check this locally by just changing that `is_listing_sync_pro_enabled` value and making sure your page still renders without any errors.
59788380-1649-u That's right. The majority of our API endpoints use cameCase for GET params, however.
59792561-1649-u We need to also check if the account group is in the US. As, currently, LSP is only available in the US.
59792578-1649-u ``` COMPLETE-FIX ```
59792731-1649-nu Is it possible for the account group to be missing any of these? Maybe using get would be better incase a key is missing.
59793685-1649-nu Ok, just as long as they keys do get passed back from the api call.
59591036-1645-u Is this not defined somewhere in the project so we don't just have a magic value?
59085166-1643-nu Are there any source we will have to add to make this work? Or have they been done already?
58709617-1639-nu Would look a bit better if this validation can be moved to the beginning of the function.
58434840-1637-u just an fyi, `APP_NAME` can't be used for the application name as it can be changed in the whitelabel 
58435137-1637-u it's probably on the handler though if the handlers use the partner_whitelabel middleware https://github.com/vendasta/MS/blob/develop/src/app/domain/middleware/partner_whitelabel.py#L49
58435420-1637-u Doesn't seem to be, right now on Test it just says "back" 
58438327-1637-u is the name set to an empty string?
58442216-1637-nu what does this do?
58444208-1637-nu thanks
58398708-1636-nu These sidebar redirect handlers are the same in rm, sm and ms, or at least should be the same. If you need to make a change here you should also make that change in the other products
58398839-1636-nu this is the `vauth` lib fyi
57779350-1625-u r'^([\w-]+(.[\w-]+)+)|(localhost)$' or r'^(\w([\w-]\*\w)\*(.\w([\w-]\*\w)\*)+)|(localhost)$' (to eliminate url like i-.am--.--.-a-url.com) Neither is perfect though...
57780346-1625-nu it still works without "www" right?
57618573-1623-nu So this function will only affect old_mappings that have no redirects? Is that on purpose?
57629602-1623-nu Should this be refactored to use our new update method? It looks like add_hostslug_msid_mapping and remove_msid_for_hostname together kind of work like our new update method.
57341398-1618-nu so these other pages are still in use?
56535931-1614-u I'd prefer ```python error_message = '{service_id}. Your account was not added properly. Please try again.'.format(service_id=service_id) ```
56536235-1614-u No semicolon needed
56388712-1613-nu ~~can you do this with ``` ReviewDocument.get_reviews ```?~~ nevermind, you can't
56389028-1613-u you can set a property on this class that defines ``` agid ``` as an argument for this api. ex ``` account_group_id = vapi.StringProperty(required=True) ``` this does a lot of serialization and boilerplate validation for you. I recommend checking out other apis that subclass vapi.ApiHandler.
56389206-1613-u I think you mean ``` GetReviewAggregate ``` ?
56389412-1613-u ``` for google ``` can mean bajillions of things in our system(s). I'd leave that part out of the docstring
56389938-1613-u i think you want to do this with knockout rather than vanilla dom manipulation. nothing technically wrong, but keeping all of our dom manipulation inside of ko view models is easier to maintain
56431219-1613-u you want to put this behind an _api route rather than a vform route. the _api routes are a little further down this file. anything on a vform route should probably subclass the VFormHandler class. also your name kwarg could just be ``` name='get-review-aggregate-api' ```. I dont think the ```-name``` part is useful anyway
56435944-1613-u id add ajax or api to this class name because python sucks and when you have 5000 dynamically typed classes its nice to have a hint about what this thing is compared to domain objects or models ie ``` GetReviewAggregateApi ``` or ``` GetReviewAggregateAjaxHandler ```
56436130-1613-u setting a content type header might take care of this json translation for you.
56436412-1613-u it may be better to do this json-ld-ifying in the template/javascript rather than here, particularly if this endpoint is ever re-used, so it can be agnostic of data scheme and pass back raw values instead of a fully formatted piece of markup which can be interpreted in other templates for other purposes
56574706-1613-u i think you probably want json. vapi will return a response formatted based on what the request is asking for so no need to set it here i think. you'd set that up in the jquery ajax configuration object. check out https://github.com/vendasta/AA/blob/develop/src/static/script/salestool/AA.ManageLeads.js#L233 for an example of an ajax configuration interfacing with a vapi endpoint that makes this relatively pain free
56575110-1613-u We should probably write some unit tests for this
56575263-1613-nu you probably want to test that the query you build in this method is scoped to the specified agid and filters out unpublished reviews
56580249-1613-nu Right. ``` scaled_stars ``` is the source-agnostic property and is normalized from 0 to 1000. You'd have to translate back to 1-5 once you got a number back though.
55857776-1606-nu could you break out the logic here into separate functions ```python if service_type == CS_KEY.FS_VENU: context = self._setup_foursuqare_context(context) elif service_type == CS_KEY.FB_PAGE: context = self._setup_facebook_context(context) ```
56174720-1606-u Quote marks for the name?
56175252-1606-nu As pointed out by Shawn, it could be better to use gritter instead of console log
56176422-1606-nu I might be too picky here, but ++index is usually used only when necessary. Use index++ if it does the same thing
56177286-1606-u There should be no change related to GMB stuffs
56193372-1606-u There should be no change related to GMB stuff
56194092-1606-nu No need to bind the $data in the click function. use bind only when you need to pass extra params except the current $data. By default, there will be two params passed to the function, the 1st one is the $data and the 2nd one is the event details.
56194783-1606-nu A loop here?
55691375-1603-u The $ here represents jQuery, because you are using jQuery to get the element, you don't need to do the checks in `isElementVerticallyInView` about whether jquery exists or if the element is a jquery element because it always will be. I would suggest passing in element[0] here then `isElementVerticallyInView` can simplified to ``` javascript var rc = element.getBoundingClientRect(); return (rc.top >= 0 && rc.bottom <= (window.innerHeight || document.documentElement.clientHeight)); ```
55692622-1603-u Can you be more explicit with the name of this function. I'm not sure what eol is supposed to stand for
55693378-1603-u It's going to be difficult to test anything that is using jquery. I'd suggest logically separating all the stuff that uses it into it's own function so that everything else can be tested easily. it might actually be better to pass the selector in to `isElementVerticallyInView` and do the jquery work in there You can then test that it was called with the correct selector. and you can mock it out so it doesn't actually run the jquery part
55708353-1603-u The usual convention is to make any variables that are a jquery selector or derivative start with a `$`. So `$element` in this case. Feel free not to do that, but it's helpful to reduce some mental load as you read the code.
55708543-1603-u Use `/* */` comments for multi-line comments (should feel familiar to your C days :smile:). I.e. ``` /* Comment comment comment */ ```
55709625-1603-u You'll want to use some kind of function throttling here, to avoid calling this function thousands of times when it isn't necessary - this function will fire off every time the window is scrolled even a single pixel, so for every pixel it moves it will be called that many times. Here's what we used in SalesTool, you could copy the scrollThrottler function verbatim and just replace the insides with your function: ```js self.scrollThrottler = function () { if (!self.scrollTimeout) { self.scrollTimeout = setTimeout(function () { self.scrollTimeout = null; if ($(window).scrollTop() === $(document).height() - $(window).height()) { if (self.nextUrl && !self.currentRequest) { // Your specific stuff should go here self.fetchSearchResults(true); // Your specific stuff should go here } // Your specific stuff should go here } }, 60); // A somewhat arbitrary number, but it works pretty well and prevents a ton of load on the browser } }; $(window).scroll(self.scrollThrottler); // This line can replace what's on line 220 of this file (the one I am commenting on) ```
55709717-1603-u Here it is in code, for context: https://github.com/vendasta/ST/blob/develop/src/static/js/manage_accounts.js#L290
55709882-1603-u It might make sense to throttle this guy too. We don't even use a resize event for this on the Manage Accounts page
55364629-1594-u I'm surprised this `render_response` call doesn't deal with context not being provided :-1:
55365221-1594-u Actually, since you're taking `context` as keyword args, it should default to an empty dictionary if you provide nothing. Python should take care of that for you: ```python def f(**kwargs): print kwargs def g(**kwargs): f(**kwargs) g() # should print '{}' ```
55532349-1589-u None of these kwargs are optional. If you make them args instead of kwargs then you don't need the check/raise ValueError stuff.
55532622-1589-u this get's the locations, not the connection
55534175-1589-u As this is now in the GoogleMyBusinessSync class it can probably be named something like `getConnectedLocation` as the GMB is already implied
55535398-1589-nu Instead of spreading out a bunch of logic to stop gmb stuff when it's disabled, can you pass this into the context and just hide it from the UI? That's generally the route we go when we put stuff behind a feature flipper. It's a lot easer to remove later as you aren't changing any of the underlying logic
55541317-1589-u if you make this `self.request.get` instead of `params.get` then you can set self.request as a dictionary in the tests and it's a lot easier to test
55541391-1589-u same with the rest of these handlers
55543130-1589-nu I don't think the account will ever have a location cursor, as that doesn't show up until our first call to get the locations
55543257-1589-nu the account won't have locations to start with anymore as that work would now have to be done in the JS
55543401-1589-nu `if (!self.locationCursor())` would work better here
55543628-1589-nu You can just `return` here, you don't need to `return true` as that implies that there could be a different return type when it's not actually going to be used for anything
55544497-1589-nu the general convention is to do this before the ajax call instead of relying on the ajax beforeSend. That being said, I'm pretty sure we did it that way because no one knew about this `beforeSend`. You might have some trouble writing a test for this, but if you can follow the same pattern for testing the success function then it should work.
55545018-1589-nu We don't want to leave console.log's in the JS as they won't be helpful to anyone (the end user won't be able to see it). We can use a $.gritter call here which will tell the user that something went wrong
55546995-1589-nu All of the operations in this function deal with the account that is passed in. Instead of passing the account in to this function, the click event should be handled by the account model. The only thing that this function can do is set the other accounts to being not selected. If you select one account then select the next one, we can leave the previous one as selected so it stays open.
55547332-1589-nu this doesn't need to be defined if there's nothing in the function
55549642-1589-nu We generally want to avoid using id's in styles as much as possible as it's hard to overwrite them. This isn't worth changing, but just for future reference if you want to style something the default should be using a class
55753655-1589-nu Ah, I thought there was collapsing
55753949-1589-nu it'd be nice to indicate that somehow, I'm not really sure the best way though