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.

Sign up for GitHub

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

Jump to bottom

[17.0][MIG] webservice: Migration to 17.0 #55

Merged
merged 50 commits into from
Sep 18, 2024
Merged

[17.0][MIG] webservice: Migration to 17.0 #55

merged 50 commits into from
Sep 18, 2024

Conversation

SilvioC2C
Copy link
Contributor

Based on #34, includes #35 and #47 forwardport

@john-herholz-dt
Copy link

Thanks for taking over. Are you able to fix the tests? Should be not so complicated

imlopes
imlopes reviewed Jul 30, 2024
View reviewed changes
webservice/tests/common.py Outdated Show resolved Hide resolved
imlopes
imlopes reviewed Jul 30, 2024
View reviewed changes
webservice/tests/common.py Outdated Show resolved Hide resolved
@imlopes
Copy link

imlopes commented Jul 30, 2024

IMHO some errors are linked to OCA/OCB@4111f02
I've tried to fix, no success

@yankinmax yankinmax force-pushed the 17.0-mig-webservice branch 4 times, most recently from c47f5dd to 7f5236e Compare August 7, 2024 10:21
@john-herholz-dt
Copy link

john-herholz-dt commented Aug 19, 2024 •
edited
Loading

Hi there, I updated my PR for this as well but was struggling with the tests.
To mock the requests is really tricky. I stopped at the point in the tests where the responses object was checked.
When the request is mocked, the responses.calls were empty.

If anybody knows how to do this, please let us know.

 responses.add(responses.GET, self.url, body="{}")
 result = self.webservice.call("get")

Here the responses.calls will be empty

@SilvioC2C SilvioC2C force-pushed the 17.0-mig-webservice branch from 7f5236e to 8c1e734 Compare August 19, 2024 19:46
@john-herholz-dt
Copy link

@SilvioC2C congrats for the passed tests. Finally.
Have you been able to run tests locally. I always struggle to run OCA tests locally. Can you describe me how to set it up?
Just a demo database?

@SilvioC2C SilvioC2C requested a review from imlopes August 19, 2024 19:49
@SilvioC2C
Copy link
Contributor Author

@john-herholz-dt @yankinmax @imlopes @gurneyalex tests are finally green, would you mind reviewing please? 🙏

@SilvioC2C
Copy link
Contributor Author

Can you describe me how to set it up? Just a demo database?

Sorry, just saw your comment.
In C2C, we use a docker image that already contains test utils: https://github.com/camptocamp/docker-odoo-project#running-tests
Usually, I just run these commands:

docker-compose run --rm --name=odoo -e DB_NAME=testdb odoo testdb-gen -i <module_name>  # this creates a new DB and install the module I want to test
docker-compose run --rm --name=odoo -e DEMO=True -e DB_NAME=testdb -e MIGRATE=False odoo odoo --workers=0 --test-enable --stop-after-init -u <module_name>  # this updates the module and runs its tests

imlopes
imlopes approved these changes Aug 19, 2024
View reviewed changes
Copy link

@imlopes imlopes left a comment

Choose a reason for hiding this comment

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

Thanks for this huge work!!!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

yankinmax
yankinmax approved these changes Aug 20, 2024
View reviewed changes
Copy link

@yankinmax yankinmax left a comment

Choose a reason for hiding this comment

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

I see where I was wrong, thanks @SilvioC2C very nice

@john-herholz-dt
Copy link

@simahawk can you merge it please? I saw you merged the migration to 16.0.
Thank you!

simahawk
simahawk requested changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@simahawk simahawk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, cannot be merged like this...
Issues:

  • commits are a bit messed up or incomplete
  • the mocking of the response seems useless as you should use responses

@SilvioC2C pls ping me and we can have a look together.

@ivantodorovich
Copy link

ivantodorovich commented Sep 3, 2024 •
edited
Loading

This PR includes:

However, that's not merged yet and by the looks of the reviewer's comments there's still some work to do there.
Could we take care of those comments and get approvals before attempting to forward port them here?

Alternatively, I'd suggest keeping the forward-port on a separate PR, so we can move on with the actual migration

oca-ci and others added 7 commits September 17, 2024 18:23
@SilvioC2C
[UPD] Update webservice.pot
1a4aa69
@OCA-git-bot @SilvioC2C
[BOT] post-merge updates
bf96745
@weblate @SilvioC2C
Update translation files
a7cb36d 
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: web-api-16.0/web-api-16.0-webservice
Translate-URL: https://translation.odoo-community.org/projects/web-api-16-0/web-api-16-0-webservice/
@mymage @SilvioC2C
Translated using Weblate (Italian)
3050fb9 
Currently translated at 100.0% (76 of 76 strings)

Translation: web-api-16.0/web-api-16.0-webservice
Translate-URL: https://translation.odoo-community.org/projects/web-api-16-0/web-api-16-0-webservice/it/
@gurneyalex @SilvioC2C
[FIX] webservice: WARNING message in logs
0836989 
The use of a compute method on ``oauth2_flow`` when this field is touched by
the server environment mixin causes it to be defined twice as computed,
with differents settings, and this ultimately causes a warning message
in the logs:

```
WARNING odoo odoo.modules.registry: webservice.backend: inconsistent 'compute_sudo' for computed fields: protocol, url, auth_type, username, password, api_key, api_key_header, oauth2_flow, oauth2_clientid, oauth2_client_secret, oauth2_token_url, oauth2_authorization_url, oauth2_audience, oauth2_scope, content_type
```

We fix this by overriding method ``compute_server_env()``
@OCA-git-bot @SilvioC2C
[BOT] post-merge updates
73a4fa5
@SilvioC2C
[IMP] webservice: pre-commit auto fixes
c750c9d
@SilvioC2C
Copy link
Contributor Author

@simahawk @ivantodorovich @imlopes @yankinmax @john-herholz-dt

I have reworked the PR to take care of the pending PR #47 (which has been superseded by #63, now merged) and the tests (but I'm still open to comments and suggestions).
I could use an helping hand about the commit history, though.

In the meanwhile, would you mind giving this PR another round of reviews? 😅

yankinmax
yankinmax approved these changes Sep 18, 2024
View reviewed changes
Copy link

@yankinmax yankinmax left a comment

Choose a reason for hiding this comment

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

Thanks @SilvioC2C , I like your changes regarding tests.

@simahawk
Copy link
Contributor

/ocabot migration webservice
/ocabot merge nobump

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Sep 18, 2024
@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-55-by-simahawk-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot mentioned this pull request Sep 18, 2024
Open
3 tasks
@OCA-git-bot OCA-git-bot merged commit 7174f50 into OCA:17.0 Sep 18, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@john-herholz-dt john-herholz-dt john-herholz-dt approved these changes

@yankinmax yankinmax yankinmax approved these changes

@ivantodorovich ivantodorovich ivantodorovich approved these changes

@simahawk simahawk simahawk approved these changes

@imlopes imlopes Awaiting requested review from imlopes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.