search

cartodb-org / cartodb

Location Intelligence & Data Visualization tool
http://cartodb.com
BSD 3-Clause "New" or "Revised" License
3 stars 2 forks source link

Merge latest blp_dev changes onto the WIP upstream merge #258

Closed tylerparsons closed 6 years ago

tylerparsons commented 6 years ago

All features introduced by https://github.com/bloomberg/cartodb/pull/229

tylerparsons commented 6 years ago

Thanks @gfiorav. I should have been clearer in my original PR description, sorry for any confusion and duplicated efforts. All the features in this PR were reviewed separately on bloomberg/cartodb, however there was a good bit of refactoring in merging the production code so I wanted to get your opinion on whether these pieces still fit well into the merge banch. Should have elaborated on the context of https://github.com/bloomberg/cartodb/pull/229

tylerparsons commented 6 years ago

@gfiorav I addressed most of those comments, agreed that these will be good improvements to the even though the code is deployed already. Regarding specs, for better or worse, these changes were rolled without them because the tests for existing models/controllers were broken. We have backlog items to fix these and write tests for this functionality as part of our next deployment.

gfiorav commented 6 years ago

Hey @tylerparsons,

Yes, don't worry! It was just a bit of miscommunication :)

I can't really tell the refactors from the final code since I'm not familiar with the original one you made. My guess is that nothing is broken, but please test it thoroughly.

Thanks

tylerparsons commented 6 years ago

Yes, manual testing has been quite thorough (painfully so) so we will definitely get those auto tests working :)