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

Add "adopt a module" message #130

Merged
merged 2 commits into from
Feb 27, 2021
Merged

Add "adopt a module" message #130

merged 2 commits into from
Feb 27, 2021

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Oct 15, 2020

See #93

legalsylvain
legalsylvain approved these changes Oct 15, 2020
View reviewed changes
Copy link
Collaborator

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

thanks !

could you check travis ?

kind regards.

@sbidoul
Copy link
Member

sbidoul commented Oct 15, 2020 •
edited
Loading

@eLBati thanks! That was quick 👍

A few comments:

  • A link to the maintainer role definition page would be useful in the comment. It should be configurable (https://odoo-community.org/page/maintainer-role by default).
  • This feature should be behind a configuration toggle as oca-github-bot is used outside of OCA (there are many examples of such toggles in the code)
  • Be careful the mention could be empty if it's a maintainer that does the PR so the implementation is probably a little bit more complex and may require some minor refactoring

@sbidoul sbidoul added the enhancement New feature or request label Nov 11, 2020
@eLBati
Copy link
Member Author

eLBati commented Jan 17, 2021

@sbidoul I made the changes. Please review. Thanks

sbidoul
sbidoul reviewed Feb 14, 2021
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Hi Lorenzo, sorry for the delay. I made a couple of suggestions to keep backward compatibility and tweak the mention a little bit. Otherwise this looks good.

@sbidoul
Copy link
Member

sbidoul commented Feb 14, 2021

Oh, and I notice these maintainer mentions does not seem to be mentioned in the readme. I can add them after merging.

@eLBati @sbidoul
Improve message
c2a818e 
Co-authored-by: Stéphane Bidoul <[email protected]>

Update environment.sample

Co-authored-by: Stéphane Bidoul <[email protected]>

compatibility with existing deployments

Co-authored-by: Stéphane Bidoul <[email protected]>
@eLBati
Copy link
Member Author

eLBati commented Feb 16, 2021

@sbidoul changes applied, thanks

@sbidoul sbidoul merged commit bca8cba into OCA:master Feb 27, 2021
@sbidoul
Copy link
Member

sbidoul commented Feb 27, 2021

@eLBati I tweaked a little bit in #139 and fixed the tests. Now I think there is one last thing to do: when a maintainer adds him/herself to an addon, the adopt mention must not appear.

@ivantodorovich ivantodorovich mentioned this pull request Jul 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbidoul sbidoul sbidoul left review comments

@legalsylvain legalsylvain legalsylvain approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eLBati @sbidoul @legalsylvain