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

12.0 mig base_dav #141

Closed
wants to merge 11 commits into from
Closed

12.0 mig base_dav #141

wants to merge 11 commits into from

Conversation

cesarlr
Copy link

@cesarlr cesarlr commented Dec 29, 2021

No description provided.

@cesarlr
Copy link
Author

cesarlr commented Dec 30, 2021

@hbrunn @fkantelberg What do you think? Ready to merge?

Copy link
Member

@fkantelberg fkantelberg left a comment

Choose a reason for hiding this comment

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

Sadly I can't test it currently. But as far as I remember the only problem was that the datetime fields changed from str to datetime objects between v11 and v12. I see that you have provided a fix there but it could be written simpler.

base_dav/models/dav_collection.py Outdated Show resolved Hide resolved
Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

you'll need to increase coverage for this to be mergeable.

I'd also suggest to update to the 3.x branch of radicale: https://radicale.org/v3.html

@cesarlr
Copy link
Author

cesarlr commented Mar 31, 2022

you'll need to increase coverage for this to be mergeable.

I'd also suggest to update to the 3.x branch of radicale: https://radicale.org/v3.html

Radicale v3 needs Python 3.6. I have had to change Python version in Travis CI because in repo it's 3.5. I don't know if it is a good idea...

config.set(section, key, data["value"])
config.set('auth', 'type', 'odoo.addons.base_dav.radicale.auth')
config.set(
'storage', 'type', 'odoo.addons.base_dav.radicale.collection'
Copy link

@oyale oyale Apr 4, 2022

Choose a reason for hiding this comment

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

It looks like we have to define the Storage class in radicale/collection.py and implement the BaseStorage methods from radicale.storage if we want to change the default values.

Otherwise, we can remove this line as the L36 loop will set the default values.

I am getting a permission error while creating directories in the default path (/var/lib/radicale).

base_dav/tests/test_base_dav.py Outdated Show resolved Hide resolved
class Rights(OwnerOnlyRights, OwnerWriteRights, AuthenticatedRights):
def authorization(self, user, path):
if path == '/':
return True
Copy link

Choose a reason for hiding this comment

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

Suggested change
return True
return ""

Copy link

Choose a reason for hiding this comment

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

This method must return granted rights (e.g. "RW"), not a boolean.

base_dav/radicale/rights.py Outdated Show resolved Hide resolved
"authenticated": AuthenticatedRights,
}.get(rights)
if not cls:
return False
Copy link

Choose a reason for hiding this comment

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

Suggested change
return False
return ""

base_dav/controllers/main.py Outdated Show resolved Hide resolved
Copy link

@oyale oyale left a comment

Choose a reason for hiding this comment

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

This will cause an error (3 instead 2 positional arguments)

base_dav/controllers/main.py Outdated Show resolved Hide resolved
@cesarlr cesarlr marked this pull request as draft April 8, 2022 16:06
@cesarlr cesarlr force-pushed the 12.0-mig-base-dav branch 2 times, most recently from 971ce14 to 5b4443b Compare April 19, 2022 16:30
@diggy128
Copy link

Anything I can do to help speed this up?
Merp from Xpansa fails to sync contacts and calendar on Android and it is essential in for implementation.

@oyale
Copy link

oyale commented May 3, 2022

Anything I can do to help speed this up? Merp from Xpansa fails to sync contacts and calendar on Android and it is essential in for implementation.

Hi @diggy128! Thanks for asking.

We need to improve code coverage to migrate the add-on into v12. Could you help us with that?

@diggy128
Copy link

diggy128 commented May 8, 2022

Hi @oyale
Writing tests is not my strong point and this time of the year I'm very time stressed but I could find some time eventually to chip in.

On the other hand, I downloaded the PR and I have come up with a lot of problems. It even seems that the module doesn't work at all.

Maybe we should look on that first or have I done something completely wrong that broke my installation?
I'll put some comments on the code to give you an insight.

@oyale
Copy link

oyale commented Jun 2, 2022

Hi @oyale Writing tests is not my strong point and this time of the year I'm very time stressed but I could find some time eventually to chip in.
Maybe we should look on that first or have I done something completely wrong that broke my installation? I'll put some comments on the code to give you an insight.

You're absolutelly right.

Sadly, we've run out of time for this issue this term. Any insights would be greatly appreciated.

@github-actions
Copy link

github-actions bot commented Oct 2, 2022

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 2, 2022
@github-actions github-actions bot closed this Nov 6, 2022
@huguesdk
Copy link
Member

i’ve done the porting to 12.0 independently in #226 and discovered this pr only after that. in that pr i’ve only ported the code to work with odoo 12, without upgrading to radicale 3. i propose to make the module already available in 12 in its current state, so that it can already be used. i think that the port to radicale 3 should be handled in a separate subsequent pr, as it is not directly linked to the porting to odoo 12. what do you think?

@oyale
Copy link

oyale commented Jul 12, 2023

I agree that handling the migration to radicale 3 in a separate PR would allow to use the module now and also, it is not a requirement to port it to odoo 12.

@diggy128
Copy link

Same thing here. Having a module that works, even with a bit of outdated dependency is better than no module at all.

@huguesdk
Copy link
Member

thanks for your answers. feel free to review #226 so that it can be merged quickly.

@fkantelberg fkantelberg mentioned this pull request Nov 1, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants