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

[MIG] web_environment_ribbon: Migration to 15.0 #2055

Merged
merged 55 commits into from
Nov 15, 2021

Conversation

ap-wtioit
Copy link
Contributor

@ap-wtioit ap-wtioit commented Oct 18, 2021

@ap-wtioit
Copy link
Contributor Author

Couldn't trigger a proper run in runbot. So i guess now it's time to wait until runbot works with 15.0.

@leemannd
Copy link

First-time contributors need a maintainer to approve running workflows. Learn more. 1 pending check
It wasn't present before. We will need help from @OCA/website-maintainers

@pedrobaeza
Copy link
Member

AFAIK, runbot is not yet working for v15, due to change to GitHub actions and the dependency to travis2docker thing.

@ap-wtioit ap-wtioit force-pushed the 15.0-mig-web_environment_ribbon branch from e2470af to 1114705 Compare October 18, 2021 13:47
@leemannd
Copy link

Hello @pedrobaeza , it is launched on other repo:

@pedrobaeza
Copy link
Member

I'm afraid not, Denis. Enter into "Details", and you'll see "Skipped" status due to what I have commented.

@leemannd
Copy link

I should have been reading better. Thank you Perdo for the informations and the time taken.

@ap-wtioit
Copy link
Contributor Author

For the pre-commit issue distutils.errors.DistutilsSetupError: Unsupported odoo version '15.0' in ./web_environment_ribbon i opened OCA/maintainer-tools#513

@ap-wtioit ap-wtioit force-pushed the 15.0-mig-web_environment_ribbon branch from 1114705 to 6d9a4bb Compare October 19, 2021 06:27
@ap-wtioit
Copy link
Contributor Author

rebased to include a652ba3 with newer setuptools-odoo that support Odoo 15.0

@simahawk simahawk changed the title 15.0 mig web environment ribbon [MIG] web_environment_ribbon: Migration to 15.0 Oct 20, 2021
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.

LG, code review only

from odoo.tests import common


class TestEnvironmentRibbonData(common.SavepointCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

SavepointCase is deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the hint, i did miss this update in the migration guide

@ap-wtioit ap-wtioit force-pushed the 15.0-mig-web_environment_ribbon branch from 6d9a4bb to 0d7fadc Compare October 20, 2021 10:04
@ap-wtioit ap-wtioit mentioned this pull request Oct 27, 2021
47 tasks
Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

Seems good. Thanks

@ph-obs
Copy link

ph-obs commented Nov 12, 2021

Do you guys know when this migration will be finished? Thanks

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-2055-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 12, 2021
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-2055-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@pedrobaeza
Copy link
Member

Very weird. Let's try again: /ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-2055-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 12, 2021
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-2055-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@ap-wtioit ap-wtioit force-pushed the 15.0-mig-web_environment_ribbon branch from 0d7fadc to fcb8c6e Compare November 15, 2021 06:55
@ap-wtioit
Copy link
Contributor Author

Better error messages would be needed i think:

+ pip install -r test-requirements.txt
ERROR: ./setup/web_environment_ribbon is not a valid editable requirement. It should either be a path to a local project or a VCS URL (beginning with bzr+http, bzr+https, bzr+ssh, bzr+sftp, bzr+ftp, bzr+lp, bzr+file, git+http, git+https, git+ssh, git+git, git+file, hg+file, hg+http, hg+https, hg+ssh, hg+static-http, svn+ssh, svn+http, svn+https, svn+svn, svn+file).

Somehow pre-commit did not create the setup directory right after the 15.0 release (when this was migrated)

@pedrobaeza
Copy link
Member

But the folder is created by pre-commit itself. Did you run it?

@ap-wtioit
Copy link
Contributor Author

ap-wtioit commented Nov 15, 2021

@pedrobaeza i ran it when migrating (there where bugs in the OCA tooling then) i ran it again, but now i get this error:

+ export SETUPTOOLS_ODOO_POST_VERSION_STRATEGY_OVERRIDE=none
+ SETUPTOOLS_ODOO_POST_VERSION_STRATEGY_OVERRIDE=none
+ touch test-requirements.txt
++ addons --addons-dir /mnt/data/odoo-data-dir --include '' --exclude '' --separator ' ' list
+ for addon in $(addons --addons-dir "${ADDONS_DIR}" --include "${INCLUDE}" --exclude "${EXCLUDE}" --separator " " list)
+ echo '-e ./setup/web_environment_ribbon'
+ for addon in $(addons --addons-dir "${ADDONS_DIR}" --include "${INCLUDE}" --exclude "${EXCLUDE}" --separator " " list)
+ echo '-e ./setup/web_m2x_options'
+ cat test-requirements.txt
-e ./setup/web_environment_ribbon
-e ./setup/web_m2x_options
+ pip install -r test-requirements.txt
Looking in indexes: https://wheelhouse.odoo-community.org/oca-simple-and-pypi
Obtaining file:///mnt/data/odoo-data-dir/setup/web_environment_ribbon (from -r test-requirements.txt (line 1))
    ERROR: Command errored out with exit status 1:
     command: /opt/odoo-venv/bin/python -c 'import io, os, sys, setuptools, tokenize; sys.argv[0] = '"'"'/mnt/data/odoo-data-dir/setup/web_environment_ribbon/setup.py'"'"'; __file__='"'"'/mnt/data/odoo-data-dir/setup/web_environment_ribbon/setup.py'"'"';f = getattr(tokenize, '"'"'open'"'"', open)(__file__) if os.path.exists(__file__) else io.StringIO('"'"'from setuptools import setup; setup()'"'"');code = f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base /tmp/pip-pip-egg-info-umhce5zy
         cwd: /mnt/data/odoo-data-dir/setup/web_environment_ribbon/
    Complete output (23 lines):
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/mnt/data/odoo-data-dir/setup/web_environment_ribbon/setup.py", line 3, in <module>
        setuptools.setup(
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools/__init__.py", line 153, in setup
        return distutils.core.setup(**attrs)
      File "/usr/lib/python3.8/distutils/core.py", line 108, in setup
        _setup_distribution = dist = klass(attrs)
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools/dist.py", line 453, in __init__
        _Distribution.__init__(
      File "/usr/lib/python3.8/distutils/dist.py", line 292, in __init__
        self.finalize_options()
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools/dist.py", line 831, in finalize_options
        ep(self)
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools/dist.py", line 852, in _finalize_setup_keywords
        ep.load()(self, ep.name, value)
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools_odoo/setup_keywords.py", line 66, in odoo_addon
        setup_keywords = prepare_odoo_addon(
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools_odoo/core.py", line 529, in prepare_odoo_addon
        addons_dir, addons_ns = _find_addons_dir()
      File "/opt/odoo-venv/lib/python3.8/site-packages/setuptools_odoo/core.py", line 335, in _find_addons_dir
        raise RuntimeError("No addons namespace found.")
    RuntimeError: No addons namespace found.

Edit:
setup/web_environment_ribbon/odoo/addons/web_environment_ribbon symlink was not commited

@ap-wtioit ap-wtioit force-pushed the 15.0-mig-web_environment_ribbon branch from 81e89bd to 387d45e Compare November 15, 2021 07:23
@pedrobaeza
Copy link
Member

I didn't get that error, so your pre-commit local installation should be damage. Sometimes pre-commit clean fixes this. Anyways, I see that you have already found the current problem, and I think it should be OK now.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-2055-by-pedrobaeza-bump-nobump, awaiting test results.

@ap-wtioit
Copy link
Contributor Author

ap-wtioit commented Nov 15, 2021

runbot still says no and gives no error message.

https://runbot.odoo-community.org/runbot/static/build/3516324-2055-387d45/logs/job_10_test_base.txt:

Step 21/22 : RUN /bin/bash -c "source ${REPO_REQUIREMENTS}/virtualenv/python3.8/bin/activate && source ${REPO_REQUIREMENTS}/virtualenv/nodejs/bin/activate && source /rvm_env.sh && /install"
 ---> Running in 816998b914d0
�[91m/bin/bash: /.repo_requirements/virtualenv/python3.8/bin/activate: No such file or directory

https://runbot.odoo-community.org/runbot/static/build/3516324-2055-387d45/logs/job_20_test_all.txt

Unable to find image 'oca-web:387d45e_env_1_job_1' locally
docker: Error response from daemon: pull access denied for oca-web, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.

Edit:
Runbot started this image: https://hub.docker.com/r/vauxoo/odoo-80-image-shippable-auto for 15.0 migration branch
Documentation says this is only suitable up to version 10.0

Base docker image for Odoo 6.1, 7.0, 8.0,9.0 and 10.0 instances

Should have been this image:
https://hub.docker.com/r/vauxoo/odoo-150-image

Where should i report this?

@OCA-git-bot OCA-git-bot merged commit 780cced into OCA:15.0 Nov 15, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e7aa0e1. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza
Copy link
Member

Runbot is not working for v15 and its replacement is runboat (note the a in between and the word game), but all of this is still a bit experimental and not officially communicated. In a nutshell, it's because the runbot we have runs at maximum Python 3.7, and Odoo 15 is working now with 3.8.

@pedrobaeza
Copy link
Member

/ocabot migration web_environment_ribbon

@ap-wtioit
Copy link
Contributor Author

@pedrobaeza thanks for your support in this one.

@pedrobaeza
Copy link
Member

@ap-wtioit it seems there's a glitch in the module: when you hover on the navbar, the ribbon disappears. Have you experimented it?

@ap-wtioit
Copy link
Contributor Author

@pedrobaeza that's this commit:
fcb8c6e
Otherwise the banner blocks the view for the first menu entry and the app menu.

@pedrobaeza
Copy link
Member

Uhm, I see it as a weird design decision. Previously there wasn't this problem, making the ribbon transparent and clickable-through it. Can this behavior be put now?

@ap-wtioit
Copy link
Contributor Author

ap-wtioit commented Jan 26, 2022

It's still click through and transparent. But it visually blocked my intention of clicking the app menu. When migrating i felt as if the banner blocked my view and i didn't expect that something that is hidden to be clickable.

image

So for me the fix was that the banner get's out of the way if i move the cursor to the menu.

@pedrobaeza
Copy link
Member

But following your own picture, that's not happening:

151122485-ced6c36c-76df-4e8b-99cb-26c75b67419c

I see this behavior as very confusing and visually not ergonomic, as well as not consistent with previous versions. Can you please revert back such change?

@ap-wtioit
Copy link
Contributor Author

i took the screenshot without the cursor hovering the menu. With hovering the menu the banner becomes invisible and the user can navigate without the banner in the way:
image

Why did you mark the main Apps menu entry as not clickable in your screenshot?

@pedrobaeza
Copy link
Member

Why did you mark the main Apps menu entry as not clickable in your screenshot?

Sorry, my mistake, but even being clickable, it's not the usual thing.

The question remains the same: hiding a big element like the ribbon when moving the cursor is not visually ergonomic, provoking eyestrain and even can be flashing epileptic warning. And it's happening on the whole navbar, even on the farthest right part. I beg you to reconsider this behavior and revert back it.

@ap-wtioit
Copy link
Contributor Author

Hiding an element when moving the cursor seems like an ok pattern to me. Regarding the flashing i can see that issue and could check for a solution.

Reverting the change can be done by anyone but it feels a bit strange to me that you seem to force me to do so.

To be clear the old implementation made it quite impossible to read some of the stuff hidden behind the banner so i tried to fix that. I still would want to work together for a solution for this issue that you cannot read stuff behind the banner and a new user would not expect it to be clickable (because visually it's not):
image

@pedrobaeza
Copy link
Member

I don't want to force you, and of course I can propose the revert myself. Sorry if you have seen my comments as a command. What I don't want is to propose such revert and don't agree on doing such move.

About hiding the apps dropdown, I don't see it as a big deal, as 99% of people will use either web_responsive or web_enterprise modules, so that dropdown is replaced by the icons view, and thus, not overlapping the ribbon.

Anyways, other option can be to change z-index for putting such dropdown on top of the ribbon.

@ap-wtioit
Copy link
Contributor Author

I got that it's not a big deal for you to hide all the items behind the banner, as i would expect you to know Odoo from the inside out.

I'm trying to make Odoo more user friendly for the people new to Odoo. And reading the menus and the headings is one of the first things a user should do to orient themselves within a new software. We also teach users to use the test system first (where we use the banner). The Banner makes all the Text behind it unreadable in terms of contrast ratios. So i tried to fix that with the current commit (that can be improved). Based on the correlation of eye movement to the cursor. I would like to work with you on improvements for the issues you have. But i think it would be best done in a new issue or pull request where we can discuss improvements.

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.