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

[IMP] web, http, http_routing: new asset for test files #33213

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@rdeodoo
Copy link
Contributor

commented May 7, 2019

Coming with odoo/enterprise#4281

This commit goal is to encapsulate every tour-test into a separate assets
bundle that would be called only during tests (command line) or by URL
(debug=tests). That way, a lot of .js files would not be loaded anymore
uselessly outside test mode and will speed up the page loads (especially in the
frontend as the backend do not reload the page anyway).

In order to do that, we needed to propagate the debug state from page to
page.
Otherwise, every page change during a test would simply lose the debug mode and
the test would stop as the test assets would not be loaded.
As we could not add the &debug=tests on every link (either hardcoded or
preprocess during the rendering), it has been decided to store it in session.

Technical summary:

  1. debug state (currently only stored in URL) will be stored in session.
    Either when adding the debug param in URL (handled with _dispatch) or by
    starting Odoo with test-enable or test-file (handled by session init).
  2. Once activated (and so set in session), debug mode will be remain activated
    even if not visible in URL (after a page navigation eg).
    To remove it, set its value to nothing, debug=. That will exit debug mode
    (whatever mode it is: debug, assets, tests).
  3. As tour-test files are now in a separate bundle, every layout (when needed)
    should t-call="web.compiled_assets_tests".
    As it would be redundant and verbose, no t-if is needed on the t-call to
    load it only in test debug mode. That will be handled by
    compiled_assets_tests that will actually do the conditionnal t-call-assets
    to web.assets_tests, if tests debug mode is activated.
  4. In addition to separating the tour-tests files in a separate bundle, we also
    moved those files to a specific folder under /static/tests/tours next to
    QUnit tests.
  5. Also, tour files will be moved in a specific folder /static/src/js/tours for
    cleanness purpose (those files will still be kept in their 'normal' assets
    as needed outside test mode since it is tours).

@robodoo robodoo added the seen 🙂 label May 7, 2019

@C3POdoo C3POdoo added the RD label May 7, 2019

@robodoo robodoo added the CI 🤖 label May 7, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch from 14c8616 to f1d733c May 7, 2019

@robodoo robodoo removed the CI 🤖 label May 7, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch 2 times, most recently from 6b055c4 to 7b39790 May 7, 2019

@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

I think I am good with this one for now.
@qsm-odoo Your input would be more that welcome, do you see any issue or unwanted behavior here?

Note that the commit about survey assets will be handled and backport by awa's team.
About the commits related to deprecated code and the x2many tour now ran without debug=assets I asked chm and wait for an answer.
Tests are now green (or were at least but can be considered as green), I just quickly created the enterprise PR that will also hopefully be green.

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch from 7b39790 to 230a6bc May 7, 2019

@robodoo robodoo added the CI 🤖 label May 7, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch from 230a6bc to 76a1166 May 8, 2019

@robodoo robodoo removed the CI 🤖 label May 8, 2019

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch from 6929d0c to 32f20c1 May 9, 2019

@qsm-odoo
Copy link
Contributor

left a comment

Just a rapid check with annoying comments, I'll check deeper once you are finished :)

Show resolved Hide resolved odoo/http.py Outdated
Show resolved Hide resolved addons/survey/views/survey_templates.xml Outdated
Show resolved Hide resolved addons/web/static/src/js/core/session.js
Show resolved Hide resolved addons/web/views/webclient_templates.xml Outdated
Show resolved Hide resolved addons/web/views/webclient_templates.xml Outdated
Show resolved Hide resolved addons/web_tour/static/src/js/tour_manager.js Outdated
Show resolved Hide resolved addons/website/controllers/main.py Outdated

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch 8 times, most recently from d3ace26 to d2c19fc May 9, 2019

@rdeodoo rdeodoo referenced this pull request May 16, 2019

Open

Tests assets next #33432

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch 4 times, most recently from fc46174 to 99d9113 May 16, 2019

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request May 16, 2019

[IMP] web, http, http_routing: new asset for test files
This commit goal is to encapsulate every tour-test into a separate assets
bundle that would be called only during tests (command line) or by URL
(debug=tests). That way, a lot of .js files would not be loaded anymore
uselessly outside test mode and will speed up the page loads (especially in the
frontend as the backend do not reload the page anyway).

In order to do that, we needed to propagate the `debug` state from page to
page.
Otherwise, every page change during a test would simply lose the debug mode and
the test would stop as the test assets would not be loaded.
As we could not add the `&debug=tests` on every link (either hardcoded or
preprocess during the rendering), it has been decided to store it in session.

Technical summary:
1. `debug` state (currently only stored in URL) will be stored in session.
   Either when adding the `debug` param in URL (handled with _dispatch) or by
   starting Odoo with `test-enable` or `test-file` (handled by session init).
2. Once activated (and so set in session), debug mode will be remain activated
   even if not visible in URL (after a page navigation eg).
   To remove it,  set its value to nothing, `debug=`. That will exit debug mode
   (whatever mode it is: debug, assets, tests).
3. As tour-test files are now in a separate bundle, every layout (when needed)
   should `t-call="web.compiled_assets_tests"`.
   As it would be redundant and verbose, no `t-if` is needed on the t-call to
   load it only in test debug mode. That will be handled by
   `compiled_assets_tests` that will actually do the conditionnal t-call-assets
   to web.assets_tests, if tests debug mode is activated.
4. In addition to separating the tour-tests files in a separate bundle, we also
   moved those files to a specific folder under /static/tests/tours next to
   QUnit tests.
5. Also, tour files will be moved in a specific folder /static/src/js/tours for
   cleanness purpose (those files will still be kept in their 'normal' assets
   as needed outside test mode since it is tours).
   This will be done in the next commit.

task-1934445
Comes with odoo/enterprise#4281
Closes odoo#33213

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request May 16, 2019

[IMP] *: move test and tour files to new folder hierarchy and asset
As we now have a new debug mode 'tests' which load a new asset bundle
containing tour-test files, we moved those files to a new folder hierarchy.
That will clean the .js files trees.
Also, those files should be included in the new asset.

Basically, the .js tour files (not test) should be inside /static/src/js/tours
while .js tour test files (test=true) should be inside /static/tests/tours next
to QUnit tests, inside a tours folder.

task-1934445
Comes with odoo/enterprise#4281
Closes odoo#33213

rdeodoo added a commit to odoo-dev/odoo that referenced this pull request May 16, 2019

[IMP] project, website_slides(_forum): no need to forward debug in URL
Now that the debug mode is stored in session (see parent commit), there is no
need anymore to forward the debug query param.

This commit clean the occurence of code that were doing that by using mainly
`keep_query`.

task-1934445
Comes with odoo/enterprise#4281
Closes odoo#33213
@Yenthe666

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

Then this is without a doubt an awesome PR. Do you have any insights or stats about the difference in size of the assets bundle? Thanks for the fast feedback.

@JKE-be

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

~6000 of lines of Js removed from assets for everyone

  • thé ability to be in debug mode persistent... what was really painful in frontend ! Now it is stored!
@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Well it will of course depend of the number of app you have installed.
I checked the branch on runbot and this is the minified test bundle content: https://pastebin.com/Zj9bLJ08

@odony odony requested review from antonylesuisse and ged-odoo May 18, 2019

@odony

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

@antonylesuisse you will want to have a look at this PR

# Store URL debug mode (might be empty) into session
if 'debug' in request.httprequest.args:
debug_mode = request.httprequest.args.get('debug')
request.session.debug = debug_mode if debug_mode in ALLOWED_DEBUG_MODES else True

This comment has been minimized.

Copy link
@odony

odony May 18, 2019

Contributor

don't alter the session unless you absolutely need it, ie. the debug mode has effectively changed

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo May 20, 2019

Author Contributor

Good catch, the if condition is now a bit more verbose but done :)

This comment has been minimized.

Copy link
@antonylesuisse

antonylesuisse May 20, 2019

Contributor

Why here ?

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo May 20, 2019

Author Contributor

I moved it from http_routing to web as you can see on this push force as this behavior is required even for non http_routing apps, else it would be inconsistent depending if the database as http_routing installed.

@@ -1049,6 +1052,9 @@ def _default_values(self):
self.setdefault("login", None)
self.setdefault("session_token", None)
self.setdefault("context", {})
# Force tests debug mode when test mode enabled to load 'assets_tests'
test_mode = odoo.tools.config['test_enable'] or odoo.tools.config['test_file']
self.setdefault("debug", 'tests' if test_mode else False)

This comment has been minimized.

Copy link
@odony

odony May 18, 2019

Contributor

You would most likely need to adapt the logic of the DisableCacheMiddleware too

@@ -64,6 +64,8 @@
# To remove when corrected in Babel
babel.core.LOCALE_ALIASES['nb'] = 'nb_NO'

ALLOWED_DEBUG_MODES = [False, None, '', '1', 'assets', 'tests']

This comment has been minimized.

Copy link
@odony

odony May 18, 2019

Contributor

It would be kind of weird to allow passing debug=1 to enable debug, but not debug=0 to disable it, don't you think? I imagine '0' could be aliased to False by _dispatch(), and '1' aliased to True?
Not sure about None, is it actually used with the current code paths? If unnecessary just omit it, so it's not tested every time.

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo May 20, 2019

Author Contributor

Indeed, I used str2bool and it will now manage such cases logically.
That allowed also to clean ALLOWED_DEBUG_MODES to only [False, True, 'assets', 'tests']

@antonylesuisse

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

I'm ok with defining assets_test, i'm against the rest, especially moving debug into session you should be able to have 2 tabs open one in debug one without.

@JKE-be

This comment was marked as resolved.

Copy link
Contributor

commented May 20, 2019

@antonylesuisse

Are you in GR ?

Let me know in which case you need to have one tab in debug and the other not?
Let me know when you are in the frontend e.g. on /shop/checkout with debug_asset=1 and want to submit a form, you don't want to keep the asset... ?

Or we can do like in backend one case by one, in this case I want to propagate the debug... It is really not a good practice imo. It is exactly like the kw in controller., when we add it, it is too late, it is because we add a bug before!

It is strange to don't have any pov from you during all the dev, to have the idea validate by chs et odoo and that after you want to remove it.

Moving debug in session is imo a real improvement and cleaning of the code.
And make the life simple for everybody.

@JKE-be

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Seems like this task was a stupid idea ... my bad!

@JKE-be JKE-be closed this May 20, 2019

@robodoo robodoo added closed 💔 and removed CI 🤖 labels May 20, 2019

@rdeodoo rdeodoo reopened this May 20, 2019

@robodoo robodoo added the CI 🤖 label May 20, 2019

};
</script>

<t t-call-assets="web.assets_common" t-css="false"/>
<t t-call-assets="point_of_sale.pos_assets_backend" t-css="false"/>
<t t-call-assets="point_of_sale.assets"/>
<t t-call="web.compiled_assets_tests"/>

This comment has been minimized.

Copy link
@antonylesuisse

antonylesuisse May 20, 2019

Contributor

optional_assets_tests ?
test_assets_tests

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo May 20, 2019

Author Contributor

@qsm-odoo Maybe you can give your input here about the template name? I think that's the nomenclature used by editor and studio for wrapper template?

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo May 20, 2019

Author Contributor

Every wrapper template currently written with _compiled prefix, see here
here
and here

I think it should remain like that for clearness purpose

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

Yes, probably not a perfect name but at least it is consistent with the rest.

This comment has been minimized.

Copy link
@JKE-be

JKE-be May 20, 2019

Contributor

final name: conditional_assets_test

+rename 3 other with the same convention

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo May 20, 2019

Author Contributor

I already renamed conditional_assets_test but I won't rename the other since they are not conditional but wrapper (no explicit group or if condition on it).
I don't like that and I prefered _compiled but it is done :)

@@ -446,8 +458,9 @@

<div t-attf-class="clearfix oe_login_buttons text-center mb-1 {{'pt-2' if form_small else 'pt-3'}}">
<button type="submit" class="btn btn-primary btn-block">Log in</button>
<t t-if="debug">
<t t-if="request.debug">

This comment has been minimized.

Copy link
@antonylesuisse

antonylesuisse May 20, 2019

Contributor

request.session

rdeodoo added some commits May 17, 2019

[IMP] web, http, http_routing: new asset for test files
This commit goal is to encapsulate every tour-test into a separate assets
bundle that would be called only during tests (command line) or by URL
(debug=tests). That way, a lot of .js files would not be loaded anymore
uselessly outside test mode and will speed up the page loads (especially in the
frontend as the backend do not reload the page anyway).

In order to do that, we needed to propagate the `debug` state from page to
page.
Otherwise, every page change during a test would simply lose the debug mode and
the test would stop as the test assets would not be loaded.
As we could not add the `&debug=tests` on every link (either hardcoded or
preprocess during the rendering), it has been decided to store it in session.

Technical summary:
1. `debug` state (currently only stored in URL) will be stored in session.
   Either when adding the `debug` param in URL (handled with _dispatch) or by
   starting Odoo with `test-enable` or `test-file` (handled by session init).
2. Once activated (and so set in session), debug mode will be remain activated
   even if not visible in URL (after a page navigation eg).
   To remove it,  set its value to nothing, `debug=`. That will exit debug mode
   (whatever mode it is: debug, assets, tests).
3. As tour-test files are now in a separate bundle, every layout (when needed)
   should `t-call="web.conditional_assets_tests"`.
   As it would be redundant and verbose, no `t-if` is needed on the t-call to
   load it only in test debug mode. That will be handled by
   `compiled_assets_tests` that will actually do the conditionnal t-call-assets
   to web.assets_tests, if tests debug mode is activated.
4. In addition to separating the tour-tests files in a separate bundle, we also
   moved those files to a specific folder under /static/tests/tours next to
   QUnit tests.
5. Also, tour files will be moved in a specific folder /static/src/js/tours for
   cleanness purpose (those files will still be kept in their 'normal' assets
   as needed outside test mode since it is tours).
   This will be done in the next commit.

task-1934445
Comes with odoo/enterprise#4281
Closes #33213
[IMP] *: move test and tour files to new folder hierarchy and asset
As we now have a new debug mode 'tests' which load a new asset bundle
containing tour-test files, we moved those files to a new folder hierarchy.
That will clean the .js files trees.
Also, those files should be included in the new asset.

Basically, the .js tour files (not test) should be inside /static/src/js/tours
while .js tour test files (test=true) should be inside /static/tests/tours next
to QUnit tests, inside a tours folder.

task-1934445
Comes with odoo/enterprise#4281
Closes #33213
[IMP] test_new_api: don't run the test in debug assets
Since we now store debug mode in session, and the session is forced to debug
tests in test mode, this tour was setting the debug mode from tests to assets
thus the test files were not loaded.

Since this debug assets was introduced in a refactoring commit and the tour
does not break without it, this commit remove it so the tour can work with
the new system.
[IMP] project, website_slides(_forum): no need to forward debug in URL
Now that the debug mode is stored in session (see parent commit), there is no
need anymore to forward the debug query param.

This commit clean the occurence of code that were doing that by using mainly
`keep_query`.

task-1934445
Comes with odoo/enterprise#4281
Closes #33213
[IMP] web, web_tour, website, point_of_sale: adapt code to new debug
As now the debug state is stored in session, the JS framework code and some
templates needed some changes.
First, the debug state is retrieved from session and not the URL anymore.
Second, the debug state does not need to be forwarded as it is now persistent.

task-1934445
Comes with odoo/enterprise#4281
Closes #33213
[IMP] web: rename file to prepare changes on next commit
As this file will be split in 2 parts, this will minimize diff.

task-1934445
Comes with odoo/enterprise#4281
Closes #33213
[IMP] mail,portal,web,web_tour,website: add debug manager in frontend
Now that the debug mode is stored in session, we need a clear indication that
debug mode is activated.
In backend, the 'fa-bug' is doing the trick but in frontend there is no visible
clue that debug mode is enabled.

This commit separate the debug manager code/feature so we can instanciate it in
frontend only with usefull options (such as enable assets, leave debug mode..).

Now, in frontend, people having access to the navbar (o_main_navbar) will see
the same 'fa-bug' icon with minimal options as mentioned above.
Also, in the footer, there will be another 'fa-bug' icon without any options.
It will be usefull when the navbar is not shown, typically the case for
public user, or for portal (without website installed)).
Clicking on it will simply leave debug mode as hinted in tooltip.

task-1934445
Comes with odoo/enterprise#4281
Closes #33213

@rdeodoo rdeodoo force-pushed the odoo-dev:master-tests-assets-latests-rde branch from b3c6f95 to 241ca05 May 20, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.