68089309-7295-nu This seems like a weird check, why is needed? Is so we don't put the zip code into `anchor_data[KEY.STATE]` 68057873-7287-nu I don't understand why we are using vdatastore stuff in this model instead of making a vobject in the vobject lib and inheriting from that 67775805-7282-nu Seems like we can move the construction of `test_facebook_post` to the setup and just change the `postedDateTime` before the put. 67755524-7281-nu I'd remove the default here if we expect this to always have a value. Better than providing an empty string where we wouldn't want one. 67759066-7281-nu +1 this will fail with a cryptic error: ``` >>> "{}{}".format("") Traceback (most recent call last): File "", line 1, in IndexError: tuple index out of range ``` 67712055-7279-nu Were we wanting to change the SDK files? 67713988-7279-nu they will just get overwritten but it is all just doc string changes anyway 67717397-7279-nu Fair! 67719415-7279-nu I would revert this file. It'll be confusing to the next person who generates the SDK when they have 18 lines they didn't know were changes. 67719620-7279-nu This is literally the 1 line that matters. 67551641-7275-nu Good job on a descriptive tag! 67551826-7275-nu You aren't passing any arguments to your deferred function. 67551931-7275-nu It would save effort to do this check before the defer. 67552369-7275-nu yeah.. I messed up hehe. I was in a bit of a rush 68099888-7275-nu I really like how you encapsulate this ? 68100273-7275-nu ? 67538347-7272-nu do we want to have a retry limit? 67539105-7272-nu or a min-backoff? or something? 67375087-7266-nu The docs state >Note that for very large time intervals (greater than 270 years on most platforms) this method will lose microsecond accuracy. So, just be aware. 67215488-7259-nu could be ```python bad_attributes = [attr for attr in data.iterkeys() if attr not in self.ATTRIBUTES] ``` 67216067-7259-nu or `set(data.iterkeys()) - set(self.ATTRIBUTES)` C-OK 67057626-7253-nu Is there any concern that a user will auth their account after exponential backoff has ramped up. Resulting in not updating the reach quickly after authenticating? 67023382-7251-nu This test will past regardless of the value of the status code we put here 66883273-7246-nu not that I'm free of these comments but this is doing more than `pre_put_hook` ;) 66883296-7246-nu missing docstring 66883310-7246-nu missing docstring 66885830-7246-nu You don't want to do this. Lets chat tomorrow 66846012-7241-nu SDK should not be in this PR Try not to generate the SDK until new endpoints are on master. 66847413-7241-nu I'll disagree here. You need to functionally test your endpoint both as a rest call, and through the sdk. You shouldn't release your sdk to VPM until the endpoints are on prod (which is what I think Brad is really getting at), but there is no reason to avoid generating it, testing with it, and getting it PR'd beforehand. 66848743-7241-nu @ckurz-va Generating it and merging it into develop will cause merge conflicts for anyone who is simultaneously adding new endpoints to be added to the SDK when they are based off master. Not the end of the world but something I like to avoid. 66856186-7241-nu It's a very simple process to manage that conflict, (Pull master into branch, regen sdk, confirm your expected changes are still there) and exposes maybe people need to chat to make sure version numbers are incrementing correctly. I personally like merge conflicts when people are working in the same space so conversations happen. As long as the merge conflict isn't spawning a TON of new work. Plus with a PR out, people who are watching the repo, which should include people currently working in the repo, will notice another team adding an endpoint, and can start the conversation on collaborating the vpm release and such before things get to master. 66867849-7241-nu is this the only place we're getting permissions and storing them? Like even on newly created FB Users we're not saving permissions? 66868117-7241-nu Also isn't there a `FacebookUserService.fetchSocialPermissions` method? 66830183-7240-nu If we are just storing these in CS is there any reason to use `vobject` and `vdatastore`? Why not just use `ndb.Model`? 66830290-7240-nu If you are just returning the class name I don't think it is necessary to implement this method. 66831414-7240-nu Why not just make this is a JsonProperty if that is what we are storing? Also are we using this for anything or storing it "just to have"? 66641991-7229-nu why the key change? 66642550-7229-nu what's the `/` for in this string? It's not a typo? 66800355-7226-nu So we have 3 different apis we have to use to interact with Infogroup? Ugh. 66498659-7221-nu ? 66509364-7221-nu Sometimes you have to be extra sure 66509896-7220-nu I don't think we need this, do we? 66511065-7220-nu Normally no but this Url Spider is doing batching like the Listing Spiders so we needed to include it here as well. 66511829-7220-nu Ah okay, I wasn't sure if this was because of the batching or just an accident. Sounds good! 66161493-7208-nu You mean `Unescapable, assuming due to '\' in review.` right? 66160792-7200-nu why caplocks in the middle? 66265868-7200-nu Should this go with the global_sources_dict spreadsheet? 66267554-7200-nu Why are we filtering listings that belong to the account_group and to the partner? Shouldn't the account_group_id be sufficient? 66271913-7200-nu why are we shortening the import? Makes it more confusing in code later. 66272638-7200-nu Do we want to protect from a large number of input docs? I can't remember the max number of entities that can be in a put_multi, but 1-1 with the input doc list could cause errors if a crazy amount of docs are pushed in. 66319830-7200-nu Where does this value of 290 come from? Some explanation should be here in the code. 66326984-7200-nu The origin of it is some formula like: Phone number = 50 marks Company Name = 50 marks address = 30 city = 10 state = 10 ... Or something like that. 66343633-7200-nu I think you're going to need these scores to factor HyperLocalListings into the equation 66366292-7200-nu Sooooo nitpicky: can you pluralize "account group" in the docstring? 66367988-7200-nu I'm pretty sure it enforces some magic (or at least it used to) while converting to and from "domain objects" (ie `Listing` vs `ListingModel`). 66454836-7200-nu What's this for? 66152684-7198-nu ? 66654628-7197-nu probably want to unit test these cases with entities where `socialPost.isError` is a factor 65715858-7187-nu Should we also be supporting `YAY`? ![yay](https://github.com/vendasta/SM/blob/develop/src/static/images/reaction-icons/yay.png) 65716351-7187-nu Nevermind http://heavy.com/tech/2016/02/yay-facebook-reactions-missing-where-why-dislike-confused-rejected-photos/ 65595297-7184-nu regarding the above comment: these definitely originated from the Urbanspoon migration 65269489-7175-nu Why is `data.get('country')` passed in twice? 65121893-7169-nu kind of boggles my mind that this test class is a child of `GaeTestCase`. I understand the word `integration` is in the class name, and if this is testing anything valuable and appengine related, maybe we keep a few, but all the cases in this class seem like perfect candidates for pure unit testing? 65108016-7164-nu O_o they give us back underscores in one of the messages? seems like something they'll fix and we'll still be erroneously checking for in a month 65105715-7163-nu i find it a little weird that deleting a review doesn't take care of deleting its child entities (comments) in a model hook. 65101175-7162-nu ? needing this decorator at all seems smelly 65091654-7160-nu The explanation here and whats in the query are a bit confusing -- we had Google+(10010) listings copied over to Google Maps(12120), and we don't want legacy ids on the Google Maps one? Shouldn't it be `src=12120` in the query? 65082457-7159-nu Did you want this to be the sandbox id or the prod id? Sandbox is 911, PROD is 915. 65082533-7159-nu Nvm. Just had to scroll down. 65082984-7159-nu Just a question: where do these magic numbers come from? And why UBERALL_PLAN_ID_AU1 is different from the one in settings.py: https://github.com/vendasta/CS/blob/ac99559bcc9b6c438fe630ab8bf678e72cda1840/src/settings.py#L700 65083053-7159-nu They are provided by uberall over their api. https://uberall.com/api/productplans 64926708-7143-nu Can't trust anything... 64775053-7140-nu Could/should we possibly do some cleanup here to prevent us from calling them again with this location id? 64659925-7134-nu Just curious about why one imports in a method like this. 64665254-7134-nu It may be a good idea to set up _retryCount 64742059-7134-nu I love this pattern so much more, good call! 64660658-7129-nu I think it's no longer "necessary". I don't think there's any place where we're actually systematically monitoring it now that prodeagle is dead. 61884755-1625-nu `account.account_group.account_group_id` is one of the best bits of code arround. 61893042-1625-nu haha is there no helper property on ``` account ``` ? this still makes me laugh though so w/e 59757758-1621-nu Why get the account_group_id off the request when it's a parameter? 59936738-1621-nu not sure how I feel about removing "ajax" from all these routes, I kind of liked that they were easy to identify 60442700-1621-nu Everything will be ajax one day 59425661-1619-nu looks good 58734224-1615-nu @utandukar-va do you know if its possible to do this direct to github so we wouldn't have to care about caching? 0_0 or maybe we can fix this in a better way 57978361-1611-nu what does * html do 57978541-1611-nu im stupid this is the generated file 56841121-1602-nu :+1: 55394450-1595-nu We should maybe have a more descriptive name for this... `is_recent_within_one_day` I guess it doesn't really matter since we only use it once and all the information is there but I don't feel like deleting everything I typed. ? 55394592-1595-nu Is using `all` overkill? I'm just wondering. 55395302-1595-nu I'd rather keep the name `is_recent`. The within one day should be obviously from the code. Also if we change what recent is (eg. a twitter lead is recent if it's within 3 days) then we don't need to worry about changing the variable name. 55396595-1595-nu I like the idea we had with location page review and comments where we made a send method on the class and then let that do the validation. I think this change can go up without it but I think at some point we should figure out how we want to structure these activities and do the same thing for each. 54252232-1588-nu The issue is that it was calculating 25MB wrong, correct? I don't think this comment quite gets that across. This is a minor change though so not a big deal. 54161741-1585-nu There's a way to call uri_for and specify scheme and netloc `uri_for('page-name', post_id=post_id, _netloc=hostname, _scheme='http')` That said, I can't remember the last time I've seen us change a URL such that having the named references is really any benefit. It's kinda nice regardless. 54171728-1585-nu Oh yeah, we aren't within webapp2 (this is running within a task), so I guess we cant (or atleast I'm not sure how we could) 52612380-1561-nu does it matter if more than one modal has the same z-index? like will every browser behave the same and show the last one in the list on top even if other overlays in the list have the same z-index? 50195956-1553-nu 0_0 Maybe add a comment? Or make a SuperAdminHandler with the explanation? This is kinda like a lie/super scary 49091207-1540-nu What does `query_deleted` mean? why would we ever use phone if its stale data? 48268336-1539-nu There's no state to this object (yet?), so I think this could be a bit nicer to read and to use (since you currently need to `new` this object to use it) by using something like the revealing module pattern. This guy makes the case for it pretty well using an angular service as an example: https://github.com/johnpapa/angular-styleguide#style-y053 ``` // Module interface return { showLeadsBadge: showLeadsBadge, ... } // Module implementation function showLeadsBadge() { ... } ``` You can flip the two sections if you can't use function declarations for some reason 48268447-1539-nu Is using an animation vs transition easier since we don't need a base class to transition from? 50041773-1539-nu This is on every page handler now? Is it pretty expensive? It'd be nice to make the compose modal as lazy loading as possible 50041861-1539-nu should this thing really be a nav ko component? 50042080-1539-nu change the name of this js model/var from DashboardViewModel to AnalyticsViewModel or something like that 50042559-1539-nu about the feature flipper... is it still needed if you are keeping the analytics page? whats the plan for it getting turned on for everyone? 50042801-1539-nu could be put into the initialization of vcompose maybe? like this only happens when you click compose post, and vcompose doesnt try to render (loading spinner thingy) until we figure out this parameter for it 50043006-1539-nu Interesting number 50044764-1539-nu typo: accoutns 50044904-1539-nu Any benefit to this being in a template? doesnt seem like it 50045588-1539-nu Whats this for? 45928044-1520-nu Does this _really_ need to be an exception? We've been talking about that at Monitoring COP lately. What's the actionable from this log item? If nothing, make this a warning or something imo. 44221615-1510-nu there is a cis-login gutter switch on prod/demo. It doesn't seem to have any conditions set? Global state with consent checkbox ticked? Either way you might be able to remove that and gutter or just not commit this line 42392621-1497-nu Was this not being used by anything? 40718983-1489-nu What's going on here? This is coming from core's API, we should be able to parse it using the regular strptime where we pass it the format. At the very least I would say that IF for some reason we need to use datetime_parser, that you should just be able to set the posted date to the result of it without recreating it... 40722368-1489-nu Fair enough. We could probably update the core API (juuuust added) to return a better formatted datetime. 39763451-1480-nu move this down 39406900-1474-nu are there 2 versions of mock in SM still? 39407088-1474-nu Ya removing it's fine. 39407144-1474-nu wait, vtest no longer contains mock, but instead has it as a dependency? 38135013-1461-nu :+1: 37653344-1455-nu Are we still `POST`ing something with this handler? I thought all the Postman tests for this were `GET`s. 37655417-1455-nu In retrospect we could have left the old `

` tags in the page and removed this padding to get the look we wanted. I don't feel strongly about changing it though. 37656641-1455-nu I thought SM was converted to a unified colour pallet, and that these were supposed to be variables. I'm surprised to see even hex values here. 37755445-1455-nu Why are you getting only one page at a time and looping in js to get all the pages? 37360613-1443-nu dunno if this needs to be an object really... could just be module functions/constants? 37360629-1443-nu doesnt look like this is used? 37020296-1442-nu cool 37020435-1442-nu can this go on just the logout route? not sure if we want to kill the cache for all landing pages all the time 37021367-1442-nu :+1: 37021656-1442-nu would rather us not worry about the issue for now. i think we want vauth middleware to set these header policies on authed routes all the time if we truly care 35006252-1432-nu :+1: 35006424-1432-nu Where was this being called from? 35006960-1432-nu Disregard my previous comment about the token Map Reduce, might want to think about deleting the unique value here as well. 34578448-1427-nu :+1: 34584884-1427-nu :+1: 34067939-1422-nu keep reading this as ```global-warming``` 34067271-1421-nu :+1: 34068341-1421-nu I think these are good, then they can show up in ga stats 34072707-1421-nu I like the error pages being seperate 33938377-1415-nu :+1: 33872063-1412-nu Our sdk generation keeps flipping this back and forth :person_frowning: 33828262-1410-nu is there a variable with a better name for this? assigning $WHITE_FONT_COLOR to styling on a td border is pretty weird. There is $WHITE_BACKGROUND_COLOR... Is there a WHITE_BORDER_COLOR or something like that which can be used? 33828377-1410-nu same kinda comment as I had about border color, assigning $SECONDARY_FONT_COLOR to a box shadow variable is odd. Is there an equivalent variable with a better suited name already, or what do you think would be more appropriate? 33828865-1410-nu You'll have to update the vcompose library to change these as the next time we bring the updated library into SM it will over write them 33829032-1410-nu @jrolheiser-va repo for vcompose located here: https://github.com/vendasta/VA-vcomponents 33624099-1408-nu included jquery but aren't using it? 33624199-1408-nu typo ``` self. notificationBannerCookie ``` 33278732-1405-nu o_O @ github's formatting here 33278914-1405-nu this also applies to phones? 33094071-1399-nu why'd you choose 569? 33094403-1399-nu not familiar with media queries, how is this different than the above? 33095190-1399-nu ooook thanks. i missed the bracket 33095972-1399-nu I think we had these as h2 for typography styles, did the style change? 33161254-1399-nu Hmm, I think the h2 should be giving a light font weight 33359329-1398-nu I think we should change it to use vapi AjaxHandler 32971572-1396-nu I'm okay with `visiblePreviousComments` be on SocialPost. It does makes sense to have it on SocialPost to have same interface and only TwitterSocialPost is the only one using it. 32076308-1384-nu indent 32161518-1384-nu Only check postable services? 31933950-1382-nu what are your thoughts on putting this in FEC? I like the idea of hide, pull-left, pull-right utility classes 31934059-1382-nu I like the idea that it's in it's own row. It separates that from the actual template better 31767096-1376-nu hehehe 31772502-1376-nu I think that was probably me :) 31435002-1369-nu woo, love that markup 31435121-1369-nu I don't think we need versioned routes for UI. Can we just have `/public/post/details/...` 31340262-1365-nu @bbassingthwaite-va, what is this? And what happens if it's not here? Because I never wrote this before for the pipelines I made but I was able to find pipelines in the datastore. 31340919-1365-nu @bbassingthwaite-va so this method operates on two different entity groups. One is with the legacy with the parent key and the other is the migrated leads model, right? 31341214-1365-nu @bbassingthwaite-va what's the message for? 29971636-1341-nu When can this happen? 29890952-1339-nu Have `UpdateAccountGroupHandler` already been removed? 30073903-1339-nu Not a fan of this put directly in the process function (which is the view), isn't there a transactional (or in this case a function that would join the transaction) update sm account function? 29440876-1332-nu return: geopoint created from the lat/long of the account group would be more appropriate as this doesn't actually augment the account group 29441472-1332-nu hm can we use any of the various service category-ish things from the account group instead? 29441901-1332-nu where is this used? are you guys writing a mapreduce or can you explain the migration scheme? 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. 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. 61581964-1714-nu this is significantly easier to read ? 61333312-1709-nu Does it need a type or popularity? 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. 60961367-1697-nu Why did we add width here? 60961401-1697-nu and here? 60766099-1690-nu we don't have to check for empty string either? 60755659-1689-nu Could move this logic to a function and add some jasmine tests perhaps. 60736152-1688-nu If it's in sass that's the case, but inline styles trump everything that isn't marked with ```!important``` 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. 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). 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. 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]`? 60314304-1668-nu Is it ever possible to get a failure? 60315494-1668-nu LSP should probably be title cased since it's a name 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. 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. 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. 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. 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. 59763077-1649-nu `dealing with triggering hotness when user shows interest` Sounds like friday night. 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) ``` 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. 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. 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. 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. 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 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? 56388712-1613-nu ~~can you do this with ``` ReviewDocument.get_reviews ```?~~ nevermind, you can't 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) ``` 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 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? 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 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 63595346-2862-nu It'll help a lot for conversions away from Jinja templating data onto the page if we keep it separate from the javascript code. This case is pretty simple and contained so it's pretty OK like this, though. 63361850-2857-nu This is good info ? , might be good for a blog post ? 63363469-2857-nu You could always just pass this in to the calls to `_fetch_histogram_stats_async` 63363897-2857-nu I am impressed this test passed before. 63364363-2857-nu when calling a function on a mock it always succeeds and returns itself. So this wouldn't have actually 'asserted' anything. 63207085-2856-nu any reason for this to not just be in global.html ? 63208483-2856-nu SM and MS have this at the bottom of all the pages, would that not work here? screen shot 2016-05-13 at 6 02 55 pm 62860153-2847-nu This reads awkwardly. Maybe we could change the logic to if debug_flag or is_local_or_test 62722407-2844-nu Is there any way to use the query from `ReviewEmailEventsStatistics`, I'm not really sure 62712879-2839-nu Remove this comment block 62358673-2828-nu the data for this and TopReviews are the same, is it possible to refactor this to use the same function as TopReviews. then we know for sure they will always be the exact same 62254524-2822-nu This is lib code which will be overridden next time the lib is updated. You'll have to make these changes to the lib, then update the lib in SR to resolve this issue. COMPLETE-FIX 62105033-2819-nu I removed `graphsInitializedWithData ` and replace it with `allDataAquired`, we'll have to watch that in the mregeds 62072587-2809-nu It's probably worth putting this explanation in the docstring 62073070-2809-nu This line made me think "there must be a better way to round a float to 1 decimal in python". But apparently there isn't .... 61972067-2806-nu We might not need this computed property cause summing up all the data seems not to make sense. BTW, I have calculate total in python domain layer. 61972094-2806-nu The same comment above. 62038489-2806-nu We might end up having an issue here. The Open Rate can be calcualted with `(sent to unique emails)/(opened by unique emails)` as the user as no way of calculating that themselves. But your PR has the total being calculated as `sent to unique emails` which would be different than the graph. And it's possible for the SMB to see the full graph and count the total themselves. I think it'd be better to sum all the data here as it'll be consistent with the graph 62038809-2806-nu does this effect other SR graphs? ~~looks like you're not using Srep.graph for the graphs here, not sure why these changes were needed~~ found the area graph 62039605-2806-nu You can probably see how inheritance would help a lot with these classes. That's what the Class.extend stuff is. inheritance.js is the lib we used for this. We're getting away from that now as ES2016 supports inheritance 62044714-2806-nu Cool, wasn't sure 61956793-2804-nu The date comes from the csv 61951067-2799-nu Not sure if we still need this import. Probably break pylint 61899268-2796-nu Is this Ajax handler just for getting reviews requested stats? If we want to use this ajax handler to get all email-related stats (reviews requested and open rate), I think we should format data as follows: ``` overall_stats = { 'quarterly': {‘reviews_requested’: {'intervals': [], 'labels': []}, ‘open_rate’: {'intervals': [], 'labels': []}}, 'monthly': ..., 'daily': ... } ``` or ``` overall_stats = { 'quarterly': { 'label': [], 'interval': { 'reviews_requested': [], 'open_rate': [] } }, 'monthly': ..., 'daily': ... } ``` 61797709-2792-nu We'll make a note to change this 61745738-2783-nu maybe we should remove this comment now as it's out-of-date 61593340-2778-nu These functions should probably just accept an account group model. The handler already has access through `hander.account.account_group` 61594362-2778-nu Does this fix that stupid graph margin problem? 61343119-2775-nu are these names backwards? seems to me the "internal" one should be the non-deprecated one 61343162-2775-nu wait... wait... I read that wrong... 61267036-2772-nu why did you cut the spinner? 61267847-2772-nu Oh, I see, the data is available on page load. What's funny is that it still takes about the same amount of time for the highchart animation (locally) as the ajax call for the Total Reviews graph 61293219-2772-nu I think it's actually a combo of the time it takes to build the chart and animate it. It might always look kinda funny. 60999670-2771-nu Does this work? Shouldn't it be ```{{ super() }}```. Not saying it's wrong, but I've never seen it like this before. 60775023-2757-nu again, is there a reason to push these objects into the list rather than just construct the list the way you want it? I'm no JS pro, so maybe there is? 60795142-2757-nu That's cool. Just curious, I thought you'd have a reason. 60466042-2755-nu ![image](https://cloud.githubusercontent.com/assets/8963131/14686516/44ced578-06f6-11e6-9415-93e9d2dc8502.png) ;) 60466143-2755-nu inline styles? 59634293-2740-nu maybe rename `path` to `actionLink` 59455346-2732-nu Since vForm inherits vApi, have you tried using `validate_arg_message` instead of `pre_process_hook` for validation? 59456704-2732-nu This is fine with me too. 59411508-2731-nu that is my goal in life 59407387-2730-nu I guess you didn't make this change ... but isn't the pythonically prefered way to cast it as str? 59407529-2730-nu pretty slick 59395576-2728-nu loooove these naming conventions /sarcasm 59296394-2727-nu Seems that these things are unnecessary or could use the config values from above? 59296610-2727-nu :no_good: 59438068-2727-nu Nice. 59269091-2726-nu Why not delete the get handler you've killed? 59378164-2726-nu Might be easier to generate the HTML on the frontend with a couple functions that take parameters from the server-side rendering context? I'm not particularly against this though. 58944060-2725-nu Does `visible: !postedByOwner` mean they can't delete their own comments? Don't we want to allow that? 58944406-2725-nu It adds a lot of complexity with regards to the responded by owner stuff. We aren't supporting it right now, I'm sure if people ask for it, that could be changed in the future. 58945551-2725-nu Also we decided that, unless they really fuck up, they can just edit their comment. Seems like that would be the better solution anyway. 58946786-2725-nu Out of curiosity what makes it more complicated? Is it concierge? Just wondering because it may not happen much, but I had one issue on black ops to delete a comment because the owner posted "I'm so sorry for your bad experience" on the wrong review, which was a 5 star good review haha 58949189-2725-nu you're required review here, but I don't think it's ever used 58949549-2725-nu nm, I found it 58432018-2715-nu this technically works cause it will kill the tests if there's an exception, but could you change it to be an assert ``` try send_activity({}) except Exception: self.fail() ``` just so it's written more like a test 58432070-2715-nu and can you change it to 4xx err instead of 400, because technically this was 404 58393977-2713-nu This should probably be done in app/domain/repcore/sources.py in `get_all_sources_as_dict` I believe you can strip it out in that function just like it strips out the listing distribution sources 58396549-2713-nu Please don't blindly make that change. That function is used to do stuff with reviews. And we still want Location Page Reviews. 58399002-2713-nu ok, sounds good to me 57234224-2702-nu That's pretty slick. 57204458-2698-nu Why is all of this in the PR? Was this intentional? 57030926-2695-nu I thought this was being used, that's why I left it 56997764-2694-nu this is a weird change, what happened here? 56997980-2694-nu NM figured it out 57387823-2688-nu Any reason why you're popping all over the place? 56527482-2674-nu It might be nice to link to the source here: `{source_name}` Here and above for the "Accurate listing" message too. I assume you have access to that information in the API call to repcore. 56527669-2674-nu Nice refactors! 56528543-2674-nu It seems a little odd to use the `handle-pubsub` queue here considering this is coming from a direct API call, not pubsub. But I also understand why you did it this way; the other notification activity probably already use this queue. :disappointed: 56531176-2674-nu :thumbsup: Glad we'll have more consistency between the different activities. 56532002-2674-nu Since we can already click on the icon and we have a 'view' link that the user can click I'm not sure if we'd want a third spot for them to click. I wonder if having three places to click or taking away one of the other places would be better UX. 56533194-2674-nu Good points. We can think about this for a future iteration. 56398758-2672-nu why is {'None': True} necessary? Seems a bit odd to me. 56399436-2672-nu Fair enough 56229497-2667-nu just syntax but you could do ```if not anchor_data_match[key].get(CS_API_KEY.LISTING_MATCH_FLAG): do something``` 56229593-2667-nu ```assertTrue``` and ```assertFalse``` might be better then ```assertEquals`` 56161440-2665-nu dang, I thought this was fartbasic.css 55608340-2661-nu just curious, is there a reason not to use `if include_only_sources and source_id not in include_only_sources` 55680730-2661-nu Not sure I follow... `[]` would also be falsy. 55052317-2649-nu Aw man, I remember when I wrote the code this way, originally. Good times. 54895331-2642-nu I wonder if there is some way for us to track who is using the iFrame.. If usage drops to 0 for a good period of time, we might be able to remove it. 54895518-2642-nu If I'm not mistaken, this is for the write a review which will still be displayed in an iframe.