-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
[8.0][FIX] Issue category for tasks #59
[8.0][FIX] Issue category for tasks #59
Conversation
Hey @franksongfeng, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
* Alex Duan<alex.duan@elico-corp.com> | ||
* Xie XiaoPeng<xie.xiaopeng@elico-corp.com> | ||
* Victor M. Martin <victor.martin@elico-corp.com> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franksongfeng include yourself as a contributor
@franksongfeng Please have a check on the comments. Travis is not working, could you fix it? |
CLA fixed (eCLA Elico) |
Any news? |
@elicoidal Here @franksongfeng there is not too much he could do. First depends on #42 be fixed. Later need to rebase this one and see if any issue. Here is a Trace:
|
@elicoidal This travis depends on merge #42 and later rebase. Then hopefully Travis will be green. @sudhir-serpentcs please could you do rebase and commit also with UT for |
@victormartinelicocorp #42 travis is already green. Once it is merged, the owner of the PR can rebase this PR. |
@sudhir-serpentcs my current problem with #42 is that it might imply a change in the current workflow name. I am in favor of it but it implies to manage properly the migration which is not yet covered by the module. Still your point is valid :) |
Hey @franksongfeng, Appreciation of efforts, |
@sudhir-serpentcs |
@elicoidal Will update you soon on same. |
@sudhir-serpentcs Any news? |
@victormartinelicocorp @elicoidal #42 is merged. Let me know if I need to do anything for this PR. |
@sudhir-serpentcs Good news indeed is the way @franksongfeng Please rebase this PR cc @elicoidal |
Inherit _prepare_project_task of the model 'br.generate.projects' and add updating task's 'categ_ids' when creating it.
@moylop260 @gurneyalex @dreispt Could give a hand? Here @franksongfeng has rebased this PR but as Travis test shows still fail on when run test over dependency module: 2017-03-29 05:33:21,511 5392 INFO openerp_test openerp.addons.business_requirement_deliverable_project.tests.test_br: test_br_wizard_apply (openerp.addons.business_requirement_deliverable_project.tests.test_br.BusinessRequirementTestCase) On the latest merge regards this module was #42 |
def _prepare_project_task(self, line, project_id): | ||
vals = super(BrGenerateProjectsWithCategories, self) \ | ||
._prepare_project_task(line, project_id) | ||
vals.update({'categ_ids': [(6, 0, [line.task_categ_id.id])]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe your error is about not validated Null
case value in the list
You could use new api feature: vals.update({'categ_ids': line.task_categ_id.ids})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we don't need a module just to update a dict values
Maybe you could use a parameter plus a if
from original module instead. It in order to avoid many many modules with small small features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moylop260 the Categories are in this module and necessary for anybody (so insulated here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moylop260 Sounds that only we are updating a dict
but what we pursue is modularity and low impact dependency.
By the way @elicoidal as soon is merged OCA/project#239 I would propose a new module
business_requirement_delivarable_main_categ
and
business_requirement_delivarable_project_main_categ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe your error is about not validated Null case value in the list
You could use new api feature: vals.update({'categ_ids': line.task_categ_id.ids})
@moylop260 Thanks we'll try
@franksongfeng Could you try to fix
cc @elicoidal |
@victormartinelicocorp Maybe you can do it. |
I'll do |
@victormartinelicocorp any news on this PR? |
Superseded by #109 |
Inherit _prepare_project_task of the model 'br.generate.projects' and add updating task's 'categ_ids' when creating it.