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

[MIG] mrp_analytic: Migration to 11.0 #186

Merged
merged 7 commits into from
Oct 11, 2018
Merged

[MIG] mrp_analytic: Migration to 11.0 #186

merged 7 commits into from
Oct 11, 2018

Conversation

SalahAdDin
Copy link
Contributor

No description provided.

@pedrobaeza
Copy link
Member

It seems you haven't used properly the migration method, as there are other modules included in this PR: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-11.0

@SalahAdDin SalahAdDin changed the title 🎉 [MIG] mrp_analytic: Migration to 11.0 [MIG] mrp_analytic: Migration to 11.0 Aug 6, 2018
@pedrobaeza
Copy link
Member

Using the method exposed, you don't need to remove the other addons, and it's the proper way to do it. Please try.

@SalahAdDin SalahAdDin mentioned this pull request Aug 6, 2018
Closed
28 tasks
@SalahAdDin
Copy link
Contributor Author

I tried, it didn't merge, i get many errors.

git am --continue
Aplicando: Move my module account_analytic_required from extra-addons to account-analytic
Sin cambios - olvidaste usar 'git add'?
Si no hay nada en el área de stage, las posibilidad es que algo mas
ya haya introducido el mismo cambio; tal vez quieras omitir este parche.
Cuando haya resuelto este problema, ejecute "git am --continue".
Si prefieres saltar este parche, ejecuta "git am --skip".
Para restaurar la rama original y detener el parchado, ejecutar "git am --abort".

@pedrobaeza
Copy link
Member

You have to try to resolve conflicts.

@SalahAdDin
Copy link
Contributor Author

Must i to resend this pull request?

@pedrobaeza
Copy link
Member

You can use locally the same branch name and forced push your new commits without problems.

@SalahAdDin
Copy link
Contributor Author

Done.

@pedrobaeza pedrobaeza added this to the 11.0 milestone Aug 6, 2018
@pedrobaeza
Copy link
Member

Great, you did it! Now I let space for the reviewers.

@SalahAdDin
Copy link
Contributor Author

Thank you.

@SalahAdDin
Copy link
Contributor Author

Hi, i'm testing that module, i'd created many manufacturing orders with respective analytic accounts but i am not getting the correct productions' number:
deepin-screen-recorder_select area_20180904115412

Why?

@SalahAdDin
Copy link
Contributor Author

🆙 👍

@SalahAdDin
Copy link
Contributor Author

I cannot understand what is the problem with the flake8's test.

AaronHForgeFlow
AaronHForgeFlow reviewed Oct 1, 2018
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

======== Testing test_flake8 ========
mrp_analytic/models/analytic_account.py:4:1: F401 'odoo.api' imported but unused
mrp_analytic/models/analytic_account.py:12:80: E501 line too long (88 > 79 characters)

@SalahAdDin
Copy link
Contributor Author

The final error, i cannot understand it.

espo-tony
espo-tony reviewed Oct 1, 2018
View reviewed changes
mrp_analytic/models/analytic_account.py Outdated

def _compute_num_productions(self):
for analytic_account in self:
analytic_account.num_productions = self.env['mrp.production'].search_count([
Copy link

Choose a reason for hiding this comment

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

Travis says this line is too long. Find a way to write it within 80 characters, either splitting it on multiple lines or (best way, IMHO) renaming analytic_account into something shorter (an_acc or record should make the job)

espo-tony
espo-tony suggested changes Oct 1, 2018
View reviewed changes
mrp_analytic/README.rst Outdated

.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/129/8.0
Copy link

Choose a reason for hiding this comment

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

Please, enter 11.0 runbot, not 8.0

@SalahAdDin
Copy link
Contributor Author

@espo-tony Ok, i did it, but i am still getting an error.

espo-tony
espo-tony reviewed Oct 2, 2018
View reviewed changes
mrp_analytic/models/analytic_account.py Outdated
# License AGPL-3 - See http://www.gnu.org/licenses/agpl-3.0.html

from odoo import models
from odoo import fields
Copy link

Choose a reason for hiding this comment

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

Please, put the 2 imports above on the same line

Copy link

Choose a reason for hiding this comment

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

@SalahAdDin Thanks. Travis is not failing anymore. Runbot failing is something different, don't know exactly what's wrong with it but I suspect it doesn't directly depends from your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, put the 2 imports above on the same line

Must i to do this?

Copy link

Choose a reason for hiding this comment

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

Yeah, it doesn't really change anything, but it's required to be compliant with OCA guidelines.

from odoo import fields, models

@SalahAdDin
Copy link
Contributor Author

@espo-tony @aheficent Thank you very much for your reviews, in hour instance we need this module very much.

Thank you.

AaronHForgeFlow
AaronHForgeFlow approved these changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

@SalahAdDin Thanks to you!. Testing on runbot and working fine. 👍

@AaronHForgeFlow
Copy link
Contributor

a nice module also would be to pass the analytic accounts to the stock moves when the stock_analytic module is merged

@SalahAdDin
Copy link
Contributor Author

SalahAdDin commented Oct 2, 2018 •
edited
Loading

@aheficent I would take more time to add that feature, we can put it as a TODO task, and i can try it after, because right now i don't know how can i do that XD.

espo-tony
espo-tony approved these changes Oct 2, 2018
View reviewed changes
Copy link

@espo-tony espo-tony left a comment

Choose a reason for hiding this comment

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

@SalahAdDin , Thank you for your work. This looks good to me 👍

@AaronHForgeFlow
Copy link
Contributor

@SalahAdDin Sure. It is just an idea, I will not work on that now but maybe in the following weeks. Stock_analytic is still not in 11.0 anyway

@SalahAdDin
Copy link
Contributor Author

Thank you very much, just merging left.

@AaronHForgeFlow
Copy link
Contributor

Can you squash your commits into one before?

@SalahAdDin
Copy link
Contributor Author

@aheficent Last commits?

@AaronHForgeFlow
Copy link
Contributor

The ones of this migration. From [MIG] mrp_analytic: Migration to 11.0 onwards. I think that having just one with [MIG] mrp_analytic: Migration to 11.0 is ok.

@SalahAdDin SalahAdDin force-pushed the 11.0-mig-mrp_analytic branch 2 times, most recently from 1c53b06 to d42a54d Compare October 2, 2018 09:35
@SalahAdDin
Copy link
Contributor Author

Done :D

@AaronHForgeFlow
Copy link
Contributor

Waiting for CI :)

@SalahAdDin
Copy link
Contributor Author

SalahAdDin commented Oct 9, 2018 •
edited
Loading

@moylop260 Hi man, i'm sorry but i need your help.
We have a problem here with runbot.

@SalahAdDin
Copy link
Contributor Author

The other module is waiting for this module: OCA/manufacture#296 (comment)

@moylop260 moylop260 merged commit 5c6e3c2 into OCA:11.0 Oct 11, 2018
@SalahAdDin SalahAdDin deleted the 11.0-mig-mrp_analytic branch October 11, 2018 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@espo-tony espo-tony espo-tony approved these changes

@AaronHForgeFlow AaronHForgeFlow AaronHForgeFlow approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
11.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@SalahAdDin @pedrobaeza @AaronHForgeFlow @espo-tony @moylop260 @oca-transbot @sbidoul @oca-travis