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

[16.0][MIG] mail_activity_done: Migration to 16.0 #1168

Merged
merged 18 commits into from
Sep 27, 2023

Conversation

amkarthik
Copy link
Member

This is a continuation of #1059 and it contains a fix reported on the parent pull request.

@amkarthik
Copy link
Member Author

Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

Nice! Reviewed and working properly now, thanks!

@rven
Copy link
Contributor

rven commented Jun 21, 2023

What about this change?
#1011
to remove the ugly monkey patching part

@amkarthik
Copy link
Member Author

amkarthik commented Jun 26, 2023

@rven
#1011 is not yet merged.
I am planning to update it here once it is merged there.

@llabusch93
Copy link
Contributor

Hello,

why was this approved now? I did test it and it does not work...

@CasVissers-360ERP
Copy link

@llabusch93 what is not working?

@llabusch93
Copy link
Contributor

@CasVissers-360ERP

I apologize for my previous response. It seems I misunderstood the functionality of the module. Upon further investigation, I discovered that the module does indeed serve the intended purpose. Although I initially expected the activity to be displayed as an object in the chatter, it appears that the module primarily allows users to access completed activities through the technical menu.

Please disregard my previous remarks. The module works as intended.

Thank you and apologies for any confusion caused.

Cheers

@StefanRijnhart
Copy link
Member

@amkarthik It does look like #1011 is a huge improvement over the monkeypatch. Realistically, it's not going to be merged anymore in that old release. Introducing it in a new release seems more appropriate to be honest. Please review that PR and include it here if you think it's any good.

@dariodelzozzo
Copy link

@amkarthik News about this PR?

Can you include the improvement mentioned by @StefanRijnhart?

@amkarthik It does look like #1011 is a huge improvement over the monkeypatch. Realistically, it's not going to be merged anymore in that old release. Introducing it in a new release seems more appropriate to be honest. Please review that PR and include it here if you think it's any good.

@StefanRijnhart
Copy link
Member

@amkarthik OK, let's continue without the improvement from #1011. I'll prepare a PR for when this is merged. Can you squash the bugfixing commits into the migration commit to make this one ready for merge?

Changed property in test to a correct one

Changed README

Added necessary checks if module does not have an icon

Added default value to _original_module
@amkarthik
Copy link
Member Author

@StefanRijnhart Squashed the bug fix commits.

@StefanRijnhart
Copy link
Member

Thanks!

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-1168-by-StefanRijnhart-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit cd65862 into OCA:16.0 Sep 27, 2023
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@StefanRijnhart
Copy link
Member

@dariodelzozzo @rven @hbrunn #1214 is ready for review.

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.