{"action":"created","comment":{"url":"https://api.github.com/repos/CartoDB/cartodb/pulls/comments/32521708","id":32521708,"diff_hunk":"@@ -0,0 +1,94 @@\n+var $ = require('jquery');\n+var cdb = require('cartodb.js');\n+var ColumnsSelectorView = require('../columns_selector_view');\n+var TablesSelectorView = require('../tables_selector_view');\n+\n+/**\n+ * View to choose the key columns (and implicitly the right table).\n+ */\n+module.exports = cdb.core.View.extend({\n+\n+  initialize: function() {\n+    this.elder('initialize');\n+    this._initBinds();\n+  },\n+\n+  render: function() {\n+    this.clearSubViews();\n+\n+    var $el = $(\n+      this.getTemplate('common/dialogs/merge_datasets/column_merge/choose_key_columns')({\n+        leftTableName: this.model.get('leftTable').get('name')\n+      })\n+    );\n+    this._renderLeftColumns($el.find('.js-left-columns'));\n+    this._renderRightTablesSelector($el.find('.js-right-tables'));\n+    this._renderRightColumns($el.find('.js-right-columns'));\n+    this.$el.html($el);\n+\n+    return this;\n+  },\n+\n+  _initBinds: function() {\n+    this.model.get('leftColumns').bind('change:selected', this._assertIfReadyForNextStep, this);\n+    this.add_related_model(this.model.get('leftColumns'));\n+\n+    this.model.get('rightColumns').bind('change:selected', this._assertIfReadyForNextStep, this);\n+    this.model.get('rightColumns').bind('reset', this._assertIfReadyForNextStep, this);\n+    this.add_related_model(this.model.get('rightColumns'));\n+  },\n+\n+  _renderLeftColumns: function($target) {\n+    var view = new ColumnsSelectorView({\n+      el: $target,\n+      collection: this.model.get('leftColumns'),\n+      excludeFilter: this._columnsExcludeFilter,\n+      selectorType: 'radio'\n+    });\n+    view.render();\n+    this.addView(view);\n+  },\n+\n+  _renderRightTablesSelector: function($target) {","path":"lib/assets/javascripts/cartodb/common/dialogs/merge_datasets/column_merge/choose_key_columns_view.js","position":52,"original_position":52,"commit_id":"fc9a3c0c6dd589ea910ca1460bbd0c59dd8eaa93","original_commit_id":"fc9a3c0c6dd589ea910ca1460bbd0c59dd8eaa93","user":{"login":"xavijam","id":132146,"avatar_url":"https://avatars.githubusercontent.com/u/132146?v=3","gravatar_id":"","url":"https://api.github.com/users/xavijam","html_url":"https://github.com/xavijam","followers_url":"https://api.github.com/users/xavijam/followers","following_url":"https://api.github.com/users/xavijam/following{/other_user}","gists_url":"https://api.github.com/users/xavijam/gists{/gist_id}","starred_url":"https://api.github.com/users/xavijam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xavijam/subscriptions","organizations_url":"https://api.github.com/users/xavijam/orgs","repos_url":"https://api.github.com/users/xavijam/repos","events_url":"https://api.github.com/users/xavijam/events{/privacy}","received_events_url":"https://api.github.com/users/xavijam/received_events","type":"User","site_admin":false},"body":"If this function is called each render, be careful with\r\n```\r\nview.model.bind('change:tableData', this._onChangeRightTableData, this);\r\n```\r\nbecause we will create a new bind each render. So I'd add a bind and unbind methods to clean it. Models cleaning (using view.add_related_model) is only run in ```clean``` method, when view is gonna be destroyed. ```clearSubViews``` only removes sub views and not sub models.","created_at":"2015-06-16T13:46:52Z","updated_at":"2015-06-16T13:46:52Z","html_url":"https://github.com/CartoDB/cartodb/pull/4059#discussion_r32521708","pull_request_url":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059","_links":{"self":{"href":"https://api.github.com/repos/CartoDB/cartodb/pulls/comments/32521708"},"html":{"href":"https://github.com/CartoDB/cartodb/pull/4059#discussion_r32521708"},"pull_request":{"href":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059"}}},"pull_request":{"url":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059","id":37708607,"html_url":"https://github.com/CartoDB/cartodb/pull/4059","diff_url":"https://github.com/CartoDB/cartodb/pull/4059.diff","patch_url":"https://github.com/CartoDB/cartodb/pull/4059.patch","issue_url":"https://api.github.com/repos/CartoDB/cartodb/issues/4059","number":4059,"state":"open","locked":false,"title":"New merge datasets modal","user":{"login":"viddo","id":978461,"avatar_url":"https://avatars.githubusercontent.com/u/978461?v=3","gravatar_id":"","url":"https://api.github.com/users/viddo","html_url":"https://github.com/viddo","followers_url":"https://api.github.com/users/viddo/followers","following_url":"https://api.github.com/users/viddo/following{/other_user}","gists_url":"https://api.github.com/users/viddo/gists{/gist_id}","starred_url":"https://api.github.com/users/viddo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/viddo/subscriptions","organizations_url":"https://api.github.com/users/viddo/orgs","repos_url":"https://api.github.com/users/viddo/repos","events_url":"https://api.github.com/users/viddo/events{/privacy}","received_events_url":"https://api.github.com/users/viddo/received_events","type":"User","site_admin":false},"body":"Fixes #3819 \r\n\r\n![merge-column](https://cloud.githubusercontent.com/assets/978461/8165054/8bb69ec4-138d-11e5-81a5-7dd873eaf37b.gif)\r\n\r\n_Example of happy-case for a column merge_\r\n![screen shot 2015-06-15 at 17 28 34](https://cloud.githubusercontent.com/assets/978461/8165089/d08f87c2-138d-11e5-8d40-87a97c89683d.png)\r\n_Error example, reusing `ErrorDetailsView` as was implemented for importer flow._\r\n\r\nMy initial approach was to duplicate existing view/model and try to adapt it to fit with the new stuff since at first sight it appeared both had the same behavior. As it turned out that was not the case I had a really hard time to make things work… so finally I decided to take one step back and implement new views/models, and migrate over the logic/tests that still made sense.\r\n\r\nDue to this modal being very large and complicated due to all steps and states involved, I decided to focus on having a working functional modal, I created a [separate issue #4058 ](https://github.com/CartoDB/cartodb/issues/4058) to deal with implementing all the visual details and CSS that are expected for this modal to be considered done.\r\n\r\nFinally, some high-view explanations of the though behind the implementation that hopefully helps when doing the code review:\r\n- Each merge flow has a bunch of steps, and are implemented as a linked link, where each _step_ links to the next. \r\n- Each step has its own model/view couple (although some views are reused, e.g. spatial_merge and merge_step)\r\n- The merge flows are:\r\n  - Column: `choose_key_columns` => `choose_merge_columns` => `merge_step`\r\n  - Spatial: `choose_right_dataset` => `choose_merge_method` => `merge_step`\r\n- The `merge_datasets_model` coordinates the steps of going to next/prev step, by keeping a reference to `currentStep` model, and `prevSteps` which is stack of step models previously seen. Going to prev step is removing the last step from the stack.\r\n- `the _merge_datasets_view` only handles rendering the current step, and the header and footer according to the state of the `currentStep` (and listens to events and reacts accordingly).\r\n\r\nFor reference of old code look at:\r\n- [cdb.admin.MergeTablesDialog](https://github.com/CartoDB/cartodb/blob/d972f172f73b3826bbd98af968042404d50d3249/lib/assets/javascripts/cartodb/table/header/merge_tables_dialog.js)\r\n- [cdb.admin.TableColumnSelector](https://github.com/CartoDB/cartodb/blob/d972f172f73b3826bbd98af968042404d50d3249/lib/assets/javascripts/cartodb/old_common/table_column_selector.js)\r\n\r\nplease code review @xavijam , cc @saleiva ","created_at":"2015-06-15T16:13:41Z","updated_at":"2015-06-16T13:46:52Z","closed_at":null,"merged_at":null,"merge_commit_sha":"f8e213def341c95ac4d5c144511e923b6c75fb95","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059/commits","review_comments_url":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059/comments","review_comment_url":"https://api.github.com/repos/CartoDB/cartodb/pulls/comments{/number}","comments_url":"https://api.github.com/repos/CartoDB/cartodb/issues/4059/comments","statuses_url":"https://api.github.com/repos/CartoDB/cartodb/statuses/fc9a3c0c6dd589ea910ca1460bbd0c59dd8eaa93","head":{"label":"CartoDB:3819-new-merge-datasets-modal","ref":"3819-new-merge-datasets-modal","sha":"fc9a3c0c6dd589ea910ca1460bbd0c59dd8eaa93","user":{"login":"CartoDB","id":1799254,"avatar_url":"https://avatars.githubusercontent.com/u/1799254?v=3","gravatar_id":"","url":"https://api.github.com/users/CartoDB","html_url":"https://github.com/CartoDB","followers_url":"https://api.github.com/users/CartoDB/followers","following_url":"https://api.github.com/users/CartoDB/following{/other_user}","gists_url":"https://api.github.com/users/CartoDB/gists{/gist_id}","starred_url":"https://api.github.com/users/CartoDB/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CartoDB/subscriptions","organizations_url":"https://api.github.com/users/CartoDB/orgs","repos_url":"https://api.github.com/users/CartoDB/repos","events_url":"https://api.github.com/users/CartoDB/events{/privacy}","received_events_url":"https://api.github.com/users/CartoDB/received_events","type":"Organization","site_admin":false},"repo":{"id":4909355,"name":"cartodb","full_name":"CartoDB/cartodb","owner":{"login":"CartoDB","id":1799254,"avatar_url":"https://avatars.githubusercontent.com/u/1799254?v=3","gravatar_id":"","url":"https://api.github.com/users/CartoDB","html_url":"https://github.com/CartoDB","followers_url":"https://api.github.com/users/CartoDB/followers","following_url":"https://api.github.com/users/CartoDB/following{/other_user}","gists_url":"https://api.github.com/users/CartoDB/gists{/gist_id}","starred_url":"https://api.github.com/users/CartoDB/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CartoDB/subscriptions","organizations_url":"https://api.github.com/users/CartoDB/orgs","repos_url":"https://api.github.com/users/CartoDB/repos","events_url":"https://api.github.com/users/CartoDB/events{/privacy}","received_events_url":"https://api.github.com/users/CartoDB/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/CartoDB/cartodb","description":"Geospatial database for the cloud","fork":false,"url":"https://api.github.com/repos/CartoDB/cartodb","forks_url":"https://api.github.com/repos/CartoDB/cartodb/forks","keys_url":"https://api.github.com/repos/CartoDB/cartodb/keys{/key_id}","collaborators_url":"https://api.github.com/repos/CartoDB/cartodb/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/CartoDB/cartodb/teams","hooks_url":"https://api.github.com/repos/CartoDB/cartodb/hooks","issue_events_url":"https://api.github.com/repos/CartoDB/cartodb/issues/events{/number}","events_url":"https://api.github.com/repos/CartoDB/cartodb/events","assignees_url":"https://api.github.com/repos/CartoDB/cartodb/assignees{/user}","branches_url":"https://api.github.com/repos/CartoDB/cartodb/branches{/branch}","tags_url":"https://api.github.com/repos/CartoDB/cartodb/tags","blobs_url":"https://api.github.com/repos/CartoDB/cartodb/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/CartoDB/cartodb/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/CartoDB/cartodb/git/refs{/sha}","trees_url":"https://api.github.com/repos/CartoDB/cartodb/git/trees{/sha}","statuses_url":"https://api.github.com/repos/CartoDB/cartodb/statuses/{sha}","languages_url":"https://api.github.com/repos/CartoDB/cartodb/languages","stargazers_url":"https://api.github.com/repos/CartoDB/cartodb/stargazers","contributors_url":"https://api.github.com/repos/CartoDB/cartodb/contributors","subscribers_url":"https://api.github.com/repos/CartoDB/cartodb/subscribers","subscription_url":"https://api.github.com/repos/CartoDB/cartodb/subscription","commits_url":"https://api.github.com/repos/CartoDB/cartodb/commits{/sha}","git_commits_url":"https://api.github.com/repos/CartoDB/cartodb/git/commits{/sha}","comments_url":"https://api.github.com/repos/CartoDB/cartodb/comments{/number}","issue_comment_url":"https://api.github.com/repos/CartoDB/cartodb/issues/comments{/number}","contents_url":"https://api.github.com/repos/CartoDB/cartodb/contents/{+path}","compare_url":"https://api.github.com/repos/CartoDB/cartodb/compare/{base}...{head}","merges_url":"https://api.github.com/repos/CartoDB/cartodb/merges","archive_url":"https://api.github.com/repos/CartoDB/cartodb/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/CartoDB/cartodb/downloads","issues_url":"https://api.github.com/repos/CartoDB/cartodb/issues{/number}","pulls_url":"https://api.github.com/repos/CartoDB/cartodb/pulls{/number}","milestones_url":"https://api.github.com/repos/CartoDB/cartodb/milestones{/number}","notifications_url":"https://api.github.com/repos/CartoDB/cartodb/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/CartoDB/cartodb/labels{/name}","releases_url":"https://api.github.com/repos/CartoDB/cartodb/releases{/id}","created_at":"2012-07-05T15:23:45Z","updated_at":"2015-06-15T11:06:39Z","pushed_at":"2015-06-16T13:43:45Z","git_url":"git://github.com/CartoDB/cartodb.git","ssh_url":"git@github.com:CartoDB/cartodb.git","clone_url":"https://github.com/CartoDB/cartodb.git","svn_url":"https://github.com/CartoDB/cartodb","homepage":"www.cartodb.com","size":475870,"stargazers_count":827,"watchers_count":827,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":196,"mirror_url":null,"open_issues_count":603,"forks":196,"open_issues":603,"watchers":827,"default_branch":"master"}},"base":{"label":"CartoDB:master","ref":"master","sha":"c8aa0006bcabafa0b1dcae0cef7ce09550a2c991","user":{"login":"CartoDB","id":1799254,"avatar_url":"https://avatars.githubusercontent.com/u/1799254?v=3","gravatar_id":"","url":"https://api.github.com/users/CartoDB","html_url":"https://github.com/CartoDB","followers_url":"https://api.github.com/users/CartoDB/followers","following_url":"https://api.github.com/users/CartoDB/following{/other_user}","gists_url":"https://api.github.com/users/CartoDB/gists{/gist_id}","starred_url":"https://api.github.com/users/CartoDB/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CartoDB/subscriptions","organizations_url":"https://api.github.com/users/CartoDB/orgs","repos_url":"https://api.github.com/users/CartoDB/repos","events_url":"https://api.github.com/users/CartoDB/events{/privacy}","received_events_url":"https://api.github.com/users/CartoDB/received_events","type":"Organization","site_admin":false},"repo":{"id":4909355,"name":"cartodb","full_name":"CartoDB/cartodb","owner":{"login":"CartoDB","id":1799254,"avatar_url":"https://avatars.githubusercontent.com/u/1799254?v=3","gravatar_id":"","url":"https://api.github.com/users/CartoDB","html_url":"https://github.com/CartoDB","followers_url":"https://api.github.com/users/CartoDB/followers","following_url":"https://api.github.com/users/CartoDB/following{/other_user}","gists_url":"https://api.github.com/users/CartoDB/gists{/gist_id}","starred_url":"https://api.github.com/users/CartoDB/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CartoDB/subscriptions","organizations_url":"https://api.github.com/users/CartoDB/orgs","repos_url":"https://api.github.com/users/CartoDB/repos","events_url":"https://api.github.com/users/CartoDB/events{/privacy}","received_events_url":"https://api.github.com/users/CartoDB/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/CartoDB/cartodb","description":"Geospatial database for the cloud","fork":false,"url":"https://api.github.com/repos/CartoDB/cartodb","forks_url":"https://api.github.com/repos/CartoDB/cartodb/forks","keys_url":"https://api.github.com/repos/CartoDB/cartodb/keys{/key_id}","collaborators_url":"https://api.github.com/repos/CartoDB/cartodb/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/CartoDB/cartodb/teams","hooks_url":"https://api.github.com/repos/CartoDB/cartodb/hooks","issue_events_url":"https://api.github.com/repos/CartoDB/cartodb/issues/events{/number}","events_url":"https://api.github.com/repos/CartoDB/cartodb/events","assignees_url":"https://api.github.com/repos/CartoDB/cartodb/assignees{/user}","branches_url":"https://api.github.com/repos/CartoDB/cartodb/branches{/branch}","tags_url":"https://api.github.com/repos/CartoDB/cartodb/tags","blobs_url":"https://api.github.com/repos/CartoDB/cartodb/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/CartoDB/cartodb/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/CartoDB/cartodb/git/refs{/sha}","trees_url":"https://api.github.com/repos/CartoDB/cartodb/git/trees{/sha}","statuses_url":"https://api.github.com/repos/CartoDB/cartodb/statuses/{sha}","languages_url":"https://api.github.com/repos/CartoDB/cartodb/languages","stargazers_url":"https://api.github.com/repos/CartoDB/cartodb/stargazers","contributors_url":"https://api.github.com/repos/CartoDB/cartodb/contributors","subscribers_url":"https://api.github.com/repos/CartoDB/cartodb/subscribers","subscription_url":"https://api.github.com/repos/CartoDB/cartodb/subscription","commits_url":"https://api.github.com/repos/CartoDB/cartodb/commits{/sha}","git_commits_url":"https://api.github.com/repos/CartoDB/cartodb/git/commits{/sha}","comments_url":"https://api.github.com/repos/CartoDB/cartodb/comments{/number}","issue_comment_url":"https://api.github.com/repos/CartoDB/cartodb/issues/comments{/number}","contents_url":"https://api.github.com/repos/CartoDB/cartodb/contents/{+path}","compare_url":"https://api.github.com/repos/CartoDB/cartodb/compare/{base}...{head}","merges_url":"https://api.github.com/repos/CartoDB/cartodb/merges","archive_url":"https://api.github.com/repos/CartoDB/cartodb/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/CartoDB/cartodb/downloads","issues_url":"https://api.github.com/repos/CartoDB/cartodb/issues{/number}","pulls_url":"https://api.github.com/repos/CartoDB/cartodb/pulls{/number}","milestones_url":"https://api.github.com/repos/CartoDB/cartodb/milestones{/number}","notifications_url":"https://api.github.com/repos/CartoDB/cartodb/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/CartoDB/cartodb/labels{/name}","releases_url":"https://api.github.com/repos/CartoDB/cartodb/releases{/id}","created_at":"2012-07-05T15:23:45Z","updated_at":"2015-06-15T11:06:39Z","pushed_at":"2015-06-16T13:43:45Z","git_url":"git://github.com/CartoDB/cartodb.git","ssh_url":"git@github.com:CartoDB/cartodb.git","clone_url":"https://github.com/CartoDB/cartodb.git","svn_url":"https://github.com/CartoDB/cartodb","homepage":"www.cartodb.com","size":475870,"stargazers_count":827,"watchers_count":827,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":196,"mirror_url":null,"open_issues_count":603,"forks":196,"open_issues":603,"watchers":827,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059"},"html":{"href":"https://github.com/CartoDB/cartodb/pull/4059"},"issue":{"href":"https://api.github.com/repos/CartoDB/cartodb/issues/4059"},"comments":{"href":"https://api.github.com/repos/CartoDB/cartodb/issues/4059/comments"},"review_comments":{"href":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059/comments"},"review_comment":{"href":"https://api.github.com/repos/CartoDB/cartodb/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/CartoDB/cartodb/pulls/4059/commits"},"statuses":{"href":"https://api.github.com/repos/CartoDB/cartodb/statuses/fc9a3c0c6dd589ea910ca1460bbd0c59dd8eaa93"}}}}