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

[MIG] project_task_material_stock: Migration to 10.0 #288

Merged

Conversation

cubells
Copy link
Member

@cubells cubells commented Jun 6, 2017

  • Update manifest file
  • Update tests

cc @Tecnativa

@cubells cubells force-pushed the 10.0-mig-project_task_materials_stock branch from e687c63 to 9140838 Compare June 7, 2017 07:10
@cubells cubells changed the title [MIG] project_task_materials_stock: Migration to 10.0 [MIG] project_task_material_stock: Migration to 10.0 Jun 7, 2017
@rafaelbn rafaelbn added this to the 10.0 milestone Jun 8, 2017
@rafaelbn
Copy link
Member

rafaelbn commented Jun 8, 2017

Please @cubells review travis:

2017-06-07 07:28:36,528 6496 ERROR openerp_test odoo.addons.project_timesheet_currency.tests.test_multicurrency_timesheet: ERROR

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Tested in runbot:

  • Go to a task -> Edit -> go to tab materials in the task and try to add materiala -> It doesn't works

Please review

Contributors
------------

* Rafael Blasco <rafabn@antiun.com>
Copy link
Member

Choose a reason for hiding this comment

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

------------

* Rafael Blasco <rafabn@antiun.com>
* Pedro M. Baeza <pedro.baeza@serviciosbaeza.com>
Copy link
Member

Choose a reason for hiding this comment

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


* Rafael Blasco <rafabn@antiun.com>
* Pedro M. Baeza <pedro.baeza@serviciosbaeza.com>
* Carlos Dauden <carlos@incaser.es>
Copy link
Member

Choose a reason for hiding this comment

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

* Rafael Blasco <rafabn@antiun.com>
* Pedro M. Baeza <pedro.baeza@serviciosbaeza.com>
* Carlos Dauden <carlos@incaser.es>
* Sergio Teruel <sergio@incaser.es>
Copy link
Member

Choose a reason for hiding this comment

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

@cubells
Copy link
Member Author

cubells commented Jun 9, 2017

@rafaelbn fixed!

@cubells cubells force-pushed the 10.0-mig-project_task_materials_stock branch from 7eceb7a to 0859aff Compare June 9, 2017 09:38
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Functionally tested 👍
Travis still fails, please review
Thanks!

@cubells
Copy link
Member Author

cubells commented Jun 13, 2017

@rafaelbn :
travis error is due to project_timesheet_currency module. Error is not related to this migration.

@rafaelbn
Copy link
Member

I see, in #275 . Maybe is useful to ping @damdam-s @gurneyalex or @guewen to notice about travis error related to project_timesheet_currency module and the could make a PR to fix it.

About this PR, as said 👍

@cubells cubells force-pushed the 10.0-mig-project_task_materials_stock branch 2 times, most recently from 65e864b to e863b2c Compare June 14, 2017 04:40
@gurneyalex
Copy link
Member

gurneyalex commented Jun 14, 2017

@rafaelbn the error is not caused by project_timesheet_currency (Travis is green on 10.0 branch of this project).

This is a classic error which happens when in the repo you have 2 unrelated modules mod_a and mod_b. mod_b introduces a new required field on a model used by mod_a (e.g. security_lead on res.company, which in the current case is introduced by the dependency on sale_stock). mod_a does not depend on mod_b so the tests of mod_a may be run by travis before mod_b and its dependencies are loaded. However the MQT scripts have preinstalled the dependencies of all modules in the repository (in our case sale_stock). This means the new column is in database, with a NOT NULL constraint. But the default value is not provided when the record is created because sale_stock is not loaded in memory yet => broken CI.

Ways out of this:

  • make project_timesheet_currency depend on sale_stock (I'm 👎 on this)
  • split the tests in travis.yml to have the module depending on sale_stock tested separately from the ones who don't
  • change the offending tests in all modules not depending on sale_stock and creating a res.company to be decorated with at_install(False) and post_install(True) -> this will make these tests run after all the modules have been loaded.

I'm fine with the last two options, provided the change is made as part of this PR.

@pedrobaeza
Copy link
Member

Thanks for the explanation @gurneyalex.

@cubells Please use method 3 in the tests of this module.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Plus the change in the test

#. If you are a project manager, go to Project > Configuration > Stages and
check option 'Consume Material' in Task Stage to generate a stock move when
the task is in that stage.
#. Go to Project -> Configuration - > Settings and enable option
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed on 10.0?

=====

#. Go to a task, edit, and add materials to be consumed on tab "Materials".
#. Move task to an stage on consume material are activated and moves and
Copy link
Member

Choose a reason for hiding this comment

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

s/on consume material are/where "Consume material" check is

"record products spent in a Task",
"version": "10.0.1.0.0",
"category": "Project Management",
"author": "Antiun Ingeniería S.L.,"
Copy link
Member

Choose a reason for hiding this comment

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

Remove Antiun

"Tecnativa, "
"Odoo Community Association (OCA)",
"license": "AGPL-3",
"application": False,
Copy link
Member

Choose a reason for hiding this comment

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

Add 'website' key and you can remove this one.

@cubells cubells force-pushed the 10.0-mig-project_task_materials_stock branch 6 times, most recently from 3b7bbf5 to d15f05a Compare June 28, 2017 20:31
@dreispt
Copy link
Member

dreispt commented Aug 9, 2017

@pedrobaeza There is a new commit, can you check?

@pedrobaeza pedrobaeza mentioned this pull request Aug 9, 2017
27 tasks
sergio-teruel and others added 2 commits August 9, 2017 16:28
* Singleton unlink tasks
* Several suggestions
* PEP8
* Warning if invoiced lines and move to not consume stage
@pedrobaeza pedrobaeza force-pushed the 10.0-mig-project_task_materials_stock branch from d15f05a to 314f44c Compare August 9, 2017 14:33
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I have rebased the branch and everything is alright, so I merge.

@pedrobaeza pedrobaeza merged commit c151087 into OCA:10.0 Aug 9, 2017
@pedrobaeza pedrobaeza deleted the 10.0-mig-project_task_materials_stock branch August 9, 2017 15:23
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