Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] don't mess with the dataset's ids #22

Merged
merged 2 commits into from
Oct 17, 2014
Merged

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Jul 21, 2014

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c751b6d on hbrunn:7.0-lp1303829 into 7fa693d on OCA:7.0.

@guewen
Copy link
Member

guewen commented Sep 25, 2014

Do you know why this dance with the ids was there?

@hbrunn
Copy link
Member Author

hbrunn commented Sep 25, 2014

Not the faintest idea I must admit. bzr blame back then didn't give any hints unfortunately.

It was introduced here:

$bzr log -r 3518
------------------------------------------------------------
revno: 3518 [merge]
committer: Fabien Meghazi <[email protected]>
branch nick: trunk
timestamp: Wed 2012-11-28 18:13:25 +0100
message:
  [MERGE] Fix kanban dataset.ids missing when using 'Show more' button
------------------------------------------------------------
Use --include-merged or -n0 to see merged revisions.
$ bzr diff -r 3517..3518
=== modified file 'addons/web_kanban/static/src/css/kanban.css'
--- addons/web_kanban/static/src/css/kanban.css 2012-11-15 14:30:59 +0000
+++ addons/web_kanban/static/src/css/kanban.css 2012-11-27 08:49:17 +0000
@@ -279,6 +279,7 @@
 .openerp .oe_kanban_view.oe_kanban_ungrouped .oe_kanban_column .oe_kanban_record {
   display: inline-block;
   padding: 2px;
+  vertical-align: top;
   box-sizing: border-box;
   -moz-box-sizing: border-box;
   -webkit-box-sizing: border-box;

=== modified file 'addons/web_kanban/static/src/css/kanban.sass'
--- addons/web_kanban/static/src/css/kanban.sass    2012-11-15 14:30:59 +0000
+++ addons/web_kanban/static/src/css/kanban.sass    2012-11-27 08:49:17 +0000
@@ -266,6 +266,7 @@
         .oe_kanban_record
             display: inline-block
             padding: 2px
+            vertical-align: top
             box-sizing: border-box
             -moz-box-sizing: border-box
             -webkit-box-sizing: border-box

=== modified file 'addons/web_kanban/static/src/js/kanban.js'
--- addons/web_kanban/static/src/js/kanban.js   2012-11-28 14:59:41 +0000
+++ addons/web_kanban/static/src/js/kanban.js   2012-11-28 17:13:25 +0000
@@ -642,14 +642,14 @@
     },
     do_show_more: function(evt) {
         var self = this;
+        var ids = self.view.dataset.ids.splice(0);
         return this.dataset.read_slice(this.view.fields_keys.concat(['__last_update']), {
             'limit': self.view.limit,
             'offset': self.dataset_offset += self.view.limit
         }).then(function(records) {
-            records.forEach(function(r) {
-                self.view.dataset.ids.push(r.id);
-            });
+            self.view.dataset.ids = ids.concat(self.view.dataset.ids);
             self.do_add_records(records);
+            self.compute_cards_auto_height();
             return records;
         });
     },

To me that looks a lot like working around a problem in read_slice that was fixed afterwards

@gurneyalex
Copy link
Member

👍!

@max3903
Copy link
Member

max3903 commented Oct 17, 2014

👍

1 similar comment
@pedrobaeza
Copy link
Member

👍

pedrobaeza added a commit that referenced this pull request Oct 17, 2014
[FIX] don't mess with the dataset's ids
@pedrobaeza pedrobaeza merged commit 747ea8f into OCA:7.0 Oct 17, 2014
@guewen
Copy link
Member

guewen commented Oct 20, 2014

Already merged, still, thanks for the explanation @hbrunn 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants