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] web_ir_actions_act_multi #1128

Merged
merged 3 commits into from
Jan 21, 2019

Conversation

alexey-pelykh
Copy link
Contributor

No description provided.

@alexey-pelykh alexey-pelykh mentioned this pull request Dec 9, 2018
40 tasks
@alexey-pelykh
Copy link
Contributor Author

@pnajman-modoolar @simahawk @lreficent since you were involved in 11.0, please review this migration for 12.0

@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-web_ir_actions_act_multi branch 2 times, most recently from 6dabd48 to 076e081 Compare December 9, 2018 16:30
@pedrobaeza pedrobaeza added this to the 12.0 milestone Dec 9, 2018
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Can you change the name of the module to web_ir_action_act_multi (singular) and take a look at the jslint issues?

@alexey-pelykh
Copy link
Contributor Author

@tarteo re: renaming yet there are other web_ir_actions_* modules - won't this break consistency? Maybe we should postpone it until 13.0 migration?

@hbrunn
Copy link
Member

hbrunn commented Jan 8, 2019

I know the conventions say no plurals, but I think we should apply this rule with some context. I started naming those modules like that years ago because that's how the models in Odoo are called. In my opninion, we shouldn't change this.

@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-web_ir_actions_act_multi branch from 076e081 to 306d936 Compare January 8, 2019 16:39
@pedrobaeza
Copy link
Member

Yes, on guidelines it's said that names that are plurals in Odoo, should be preserved: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#11modules, so it's OK to leave it as is.

Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

👍 Thanks. code looks good. I couldn't test the functionality though. Maybe you can add demo action so it's easier to test the functionality.

@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). 🤖

@alexey-pelykh
Copy link
Contributor Author

@tarteo thanks for a hint, I'll consider adding example as a separate module. Not sure if this module is used a lot elsewhere, yet at least it's being used in this one OCA/timesheet#139

@hbrunn
Copy link
Member

hbrunn commented Jan 9, 2019

you can just add some example action as demo data, much less work and we don't clutter out repos with a lot of _demo modules.

@alexey-pelykh
Copy link
Contributor Author

@hbrunn a snippet or a link to example would be much appreciated, since I haven't yet worked with demo data.

@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-web_ir_actions_act_multi branch from 306d936 to b16c73f Compare January 9, 2019 15:46
@hbrunn
Copy link
Member

hbrunn commented Jan 9, 2019

@hbrunn
Copy link
Member

hbrunn commented Jan 9, 2019

whatever you add to the demo key in the manifest is only loaded when the demo flag for the module is set, which is never the case on production servers (if not somebody messed up majorly).

Meant to add data needed for tests, but we can use it perfectly fine to, well, demo a module's functionality.

@alexey-pelykh
Copy link
Contributor Author

@hbrunn thought about examples - this is so purely developer-related module, not even sure it's worth adding an example besides describing what this module does. If you have any particular ideas for demos - it would be great to hear them!

@hbrunn
Copy link
Member

hbrunn commented Jan 21, 2019

hmm, I just answered your question on how to do @tarteo's proposal, don't have much of an opinion about this. As everyone approved, I'll just merge.

@hbrunn hbrunn merged commit a50d9ab into OCA:12.0 Jan 21, 2019
@alexey-pelykh
Copy link
Contributor Author

@hbrunn true, sorry for mis-tagging!

@alexey-pelykh alexey-pelykh deleted the 12.0-mig-web_ir_actions_act_multi branch January 21, 2019 11:56
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.

8 participants