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

[11.0][web_widget_x2many_matrix] fix issue with limit of records #1014

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

JordiBForgeFlow
Copy link
Member

Fixes #1012

@JordiBForgeFlow
Copy link
Member Author

Thanks to @tonihr for proposing the fix!

cc @agranadosb, @simahawk @pedrobaeza

As you can see below, I can now generate huge many2many_matrix grids.

image

@pedrobaeza pedrobaeza added this to the 11.0 milestone Aug 8, 2018
@pedrobaeza
Copy link
Member

Why a new file instead existing ones?

@pedrobaeza
Copy link
Member

Check syntax recommendations from https://travis-ci.org/OCA/web/jobs/413220985#L844

@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza We create a new file so it is easier to understand that we are changing a different class.
@simahawk I have also applied the proper indentations.

@simahawk
Copy link
Contributor

simahawk commented Aug 9, 2018

I'm fine w/ the new file BTW :)

@pedrobaeza
Copy link
Member

Let's not introduce new JS lint warnings:

web_widget_x2many_2d_matrix/static/src/js/abstract_view_matrix_limit_extend.js:10: [W7903(javascript-lint), ] Unexpected tab character. [Error/no-tabs]
web_widget_x2many_2d_matrix/static/src/js/abstract_view_matrix_limit_extend.js:10: [W7903(javascript-lint), ] Mixed spaces and tabs. [Error/no-mixed-spaces-and-tabs]
web_widget_x2many_2d_matrix/static/src/js/abstract_view_matrix_limit_extend.js:12: [W7903(javascript-lint), ] Missing radix parameter. [Error/radix]
web_widget_x2many_2d_matrix/static/src/js/abstract_view_matrix_limit_extend.js:10: [W7910(wrong-tabs-instead-of-spaces), ]  Use wrong tabs indentation instead of four spaces

@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza I'm not sure where you found the warnings, but I attended them.

@pedrobaeza
Copy link
Member

You have introduced even more warnings. You can see them on https://travis-ci.org/OCA/web/jobs/423957411#L2131

@JordiBForgeFlow JordiBForgeFlow force-pushed the 11_fix_matrix_limit branch 3 times, most recently from 1dc23c3 to 554d7b1 Compare September 10, 2018 17:06
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbeficent can you get back to this? :)

@pedrobaeza
Copy link
Member

@jbeficent I have faced this issue today. Are you going to finish this?

@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza let me finish it now

@JordiBForgeFlow JordiBForgeFlow force-pushed the 11_fix_matrix_limit branch 2 times, most recently from 69b8a97 to 86f7e77 Compare November 9, 2018 09:33
@pedrobaeza
Copy link
Member

@tarteo @yajo @simahawk can you re-review?

@JordiBForgeFlow
Copy link
Member Author

@tarteo @pedrobaeza I have no idea how to fix the lint errors. I have tried to fix, but I cannot resolve. Any idea?

@tarteo
Copy link
Member

tarteo commented Nov 9, 2018

Try it like this:

odoo.define(
    "web_widget_x2many_2d_matrix.matrix_limit_extend",
    function (require) {
        "use strict";

        var AbstractView = require("web.AbstractView");

        AbstractView.include({
        ...
     }
);

@@ -0,0 +1,19 @@
odoo.define(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there's an exception on the line width for the odoo.define lines in the linter, just for the case you have at hand.

The best you can do here is to put just a single line:

 odoo.define( "web_widget_x2many_2d_matrix.matrix_limit_extend", function (require) {

Thanks to that exception, the linter should be OK.

attrs.limit = Infinity;
}
},
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing here );

@JordiBForgeFlow
Copy link
Member Author

We're good to go

@simahawk simahawk merged commit 56dbdca into OCA:11.0 Nov 9, 2018
@pedrobaeza
Copy link
Member

There were a syntax error fixed here: 899f01f

attrs.limit = Infinity;
}
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbeficent In Google Chrome this fails with an Uncaught SyntaxError: missing ) after argument list in line 15

Changing to below seems to work:

                    attrs.limit = Infinity;
                }
            },
        });
    }
);

But I'm no javascript expert!

@pedrobaeza
Copy link
Member

@hhgabelgaard I have just written the same and fixed it on the fly. Just update...

@hhgabelgaard
Copy link
Contributor

@pedrobaeza Thanks!

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

Successfully merging this pull request may close these issues.

6 participants