Mig mgmtsystem action to v. 10.0 #168

eugen-don · 2017-03-14T10:56:34Z

Mig mgmtsystem action to v. 10.0
depends:

MIG mgmtsystem_system to V. 10.0 #166 mgmtsystem

sbidoul · 2017-03-27T17:37:17Z

I think this one is the next in the dependency graph.

I relaunched travis which shows a lint error and a couple of test failures.

eugen-don · 2017-03-27T17:41:51Z

@sbidoul thanks

eugen-don · 2017-03-27T17:42:47Z

@sbidoul
the joblogs not showing since the relaunch?

edit:
Fixed linting errors in reports and tests ... rebasing on OCA/10.0

eugen-don · 2017-03-27T19:13:02Z

rebased on OCA/10.0

grindtildeath

Only code review.
Just didn't review all the new features as I don't know what it's supposed to do, and I'm not sure a migration PR is the best place for it.

grindtildeath · 2017-03-28T13:03:45Z

mgmtsystem_action/models/mgmtsystem_action.py

@@ -136,15 +136,25 @@ def create(self, vals):
        """Creation of Action."""
        Sequence = self.env['ir.sequence']
        vals['reference'] = Sequence.next_by_code('mgmtsystem.action')
-        action = super(MgmtSystemAction, self).create(vals)
-        self.send_mail_for_action(action)


Why not send the email ?

@grindtildeath
That was a sunday miskate, thanks for the spot my friend :D

Correction: This was not an mistake, this is removed because of the new feature introduced in project module in V10.0, which I adapted in this module.

Its this method: _track_template(self, tracking):
checks if stage_id has been changed and sends the mail template that is associated to the stage.

Big advantage in comparison to the old method is that you can choose which stages send notifications for actions. In the old method the notifications were allways send regardles if you wanted it or not.

grindtildeath · 2017-03-28T13:04:07Z

mgmtsystem_action/models/mgmtsystem_action.py

    def write(self, vals):
        """Update user data."""
        if vals.get('stage_id'):
-            stage_new = self._get_stage_new()


Why change method names ?

Hi @grindtildeath
I changed the methods name because stage_new() implies creating a new stage, the name does not imply on the methods intention to retrive the default stage from the database.

So if possible i would like to stay consistent to the odoo core modules with stage_default or something like get_default_stage_id so that the methods name at least contains the word default in it.
I chose _default_stage since this was the shortest method name fullfillig the requirement of not beein missleading.

/opt/odoo/10/srv/addons/crm/tests/test_lead2opportunity.py: 'stage_id': default_stage_id

grindtildeath · 2017-03-28T13:09:01Z

mgmtsystem_action/models/mgmtsystem_action.py

-            [('is_starting', '=', True)],
-            limit=1)
+        return (
+            self.env.ref('mgmtsystem_action.stage_draft', False) or


Why change the default behaviour ? I suppose we should be able not to use stage_draft as default (is_starting=True).

@grindtildeath
The prior version of the method returned the xmlid of the stage, stage_draft, meaning the stage with id stage_draft had to be present at all times in the database for the create method to work, now if a manager user deletes the stage with the id stage_draft it can no longer be retrieved by the method and create method for new records fails. So this new method checks for there being a stage with id stage_draft and if this stage is not present it returns the stage that has the bool field as being the starting stage.

grindtildeath · 2017-03-28T13:11:09Z

mgmtsystem_action/models/mgmtsystem_action.py

 from datetime import datetime, timedelta


-class MgmtSystemAction(models.Model):
+class MgmtsystemAction(models.Model):


Please don't change class names for compatibility.

@grindtildeath
Did you try it out? The modules names are seperated by a dot .

So in class MgmtsystemAction
field _name is written as name = "mgmtsystem.action"
the casecamel has to bee accordingly or it throwse an error

grindtildeath · 2017-03-28T13:11:39Z

mgmtsystem_action/models/mgmtsystem_action.py

    _inherit = ['mail.thread', 'ir.needaction_mixin']
+    _order = "date_deadline desc"
+    _track = {


What is this supposed to do ?

grindtildeath · 2017-03-28T13:18:25Z

mgmtsystem_action/models/mgmtsystem_action_stage.py

    is_ending = fields.Boolean('Ending stage')
+    sequence = fields.Integer(
+        'Sequence', default=1, help="Used to order stages. Lower is better.")


Please keep default behaviour (default=100)

grindtildeath · 2017-03-28T13:19:18Z

mgmtsystem_action/models/mgmtsystem_action_stage.py

+    case_default = fields.Boolean('Common to All Teams')
+    is_starting = fields.Boolean(
+        string='Is starting Stage',
+        help="select stis checkbox if this is the default stage \n"


"Select this checkbox if this is default stage." (it might for for something else than nonconformities)

grindtildeath · 2017-03-28T13:21:39Z

mgmtsystem_action/views/mgmtsystem_action_stage.xml

+            <field name="arch" type="xml">
+                <tree string="Action Stage">
+                    <field name="name"/>
+                    <field name="sequence" widget="handle" groups="base.group_no_one"/>


Why define this group here ?

@grindtildeath
thats also not needed, since ill be splitting migration code and new features, this will be left out as well.

Thanks again for the good review...
Also thanks to @sbidoul for spotting the linting errors and the test failures.
As for the failures im still working on it.

sbidoul · 2017-03-28T15:06:10Z

@eugen-don thanks for your work around here.

To facilitate review and accelerate the migration, perhaps could you do a first PR for the migration, and then a second one with the new features?

eugen-don · 2017-03-29T09:35:42Z

@sbidoul @grindtildeath

I see the new features cause some confusion on the review.

ILl split the code this week in 2 PRs one just the migration and an aditional one for the new features..

sbidoul · 2017-03-29T13:39:05Z

ILl split the code this week in 2 PRs one just the migration and an aditional one for the new features..

@eugen-don thanks! Let me know when it's done, it's should be mergeable quickly so we can move forward with the rest while the feature improvement is fine-tuned.

grindtildeath · 2017-03-30T08:36:47Z

@eugen-don
All right let me know when PR is split and will review one more time. Huge thanks for your work on this module by the way.

max3903

When the action is closed, all the fields should be readonly.

Otherwise 👍

eugen-don · 2017-04-20T06:04:58Z

Hi @max3903

it allready was fixed but i removed it since some people told me to put improvements in a different PR.

IMO its an improvement since in v 9.0 you can edit all fields in closed actions.

Should i first make a fix for v 9.0 and do a PR there? or else we have new code here that cant be compared against v 9.0 when reviewing...

sbidoul · 2017-04-20T11:00:09Z

@eugen-don IMO keep improvements in other PRs so we can expedite migration and start collaborating on improvements.

sbidoul · 2017-04-20T11:04:54Z

mgmtsystem_action/models/mgmtsystem_action.py

-        for action in self:
-            template.send_mail(action.id, force_send=force_send)
+        self.env['mail.template'].browse(template.id).send_mail(
+            action.id, force_send=force_send)


@eugen-don why do you rmove the for action loop?

@sbidoul Hi
The mails are send on record creation, there is no need for a looping.
I actually wanted to preserve the looping since i wasnt sure about its purpose...

So i did some extensive testin just to figure out that the looping prevented the mails from beeing send on record creation

Ah I see it receives the action as argument. Then it must be @api.model since this method does not act on a recordset.

you think we should use model here instead of multi?

yes, it's mechanical: if you have @api.multi, you must have either ensure_one or iterate over the recordset, in this case you don't so @api.model correctly expresses this is a method that does not act on a recordset of actions

Thanks... thats a good explanation... ill do a quick test and change this

grindtildeath

Just a small improvement, otherwise looks good ! Thanks again for your work.

grindtildeath · 2017-04-20T13:51:04Z

mgmtsystem_action/models/mgmtsystem_action.py

        template = self.env.ref(
            'mgmtsystem_action.email_template_new_action_reminder')
-        for action in self:
-            template.send_mail(action.id, force_send=force_send)
+        self.env['mail.template'].browse(template.id).send_mail(


As template is a recordset there's no use of browsing mail.template.
You can simply use : template.send_mail()
Otherwise it makes an unnecessary select in DB.

grindtildeath · 2017-04-20T13:51:30Z

mgmtsystem_action/models/mgmtsystem_action.py

@@ -219,5 +209,5 @@ def process_reminder_queue(self, reminder_days=10):
        template = self.env.ref(
            'mgmtsystem_action.action_email_template_reminder_action')
        for action in action_ids:
-            template.send_mail(action.id, force_send=True)
+            self.env['mail.template'].browse(template.id).send_mail(action.id)


Same as before.

@grindtildeath Thanks for the spot

Im dooing too much copy paste lately :D

MIG mgmtsystem_action to V. 10.0

sbidoul · 2017-04-20T14:27:53Z

Thanks!

eugen-don · 2017-04-20T14:39:56Z

thnaks for your support guys

max3903 modified the milestone: 10.0 Mar 18, 2017

pedrobaeza mentioned this pull request Mar 18, 2017

Migration to version 10.0 #159

Closed

31 tasks

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch 6 times, most recently from 4158120 to 444c42c Compare March 22, 2017 15:25

eugen-don changed the title ~~Mig mgmtsystem action to v. 10.0~~ WIP Mig mgmtsystem action to v. 10.0 Mar 23, 2017

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch from 444c42c to 8953c4c Compare March 25, 2017 15:06

eugen-don changed the title ~~WIP Mig mgmtsystem action to v. 10.0~~ Mig mgmtsystem action to v. 10.0 Mar 26, 2017

eugen-don mentioned this pull request Mar 26, 2017

Mig mgmtsystem nonconformity to v. 10.0 #169

Merged

3 tasks

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch 2 times, most recently from 6efe004 to d6fd8c7 Compare March 27, 2017 08:57

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch 2 times, most recently from c0f0e73 to 92b8004 Compare March 27, 2017 17:52

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch 2 times, most recently from 5e8db9a to 7f2b270 Compare March 27, 2017 20:03

grindtildeath reviewed Mar 28, 2017

View reviewed changes

eugen-don changed the title ~~Mig mgmtsystem action to v. 10.0~~ WIP Mig mgmtsystem action to v. 10.0 Mar 29, 2017

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch from d15350a to fece5c2 Compare April 1, 2017 10:27

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch 2 times, most recently from d7129f8 to 7b8e6ff Compare April 18, 2017 09:10

eugen-don changed the title ~~WIP Mig mgmtsystem action to v. 10.0~~ Mig mgmtsystem action to v. 10.0 Apr 18, 2017

max3903 self-requested a review April 20, 2017 01:32

max3903 requested changes Apr 20, 2017

View reviewed changes

sbidoul reviewed Apr 20, 2017

View reviewed changes

max3903 approved these changes Apr 20, 2017

View reviewed changes

grindtildeath reviewed Apr 20, 2017

View reviewed changes

MIG mgmtsystem_action to V. 10.0

8ee1cbf

MIG mgmtsystem_action to V. 10.0

eugen-don force-pushed the MIG-mgmtsystem_action-to-V.-10.0 branch from d563ce2 to 8ee1cbf Compare April 20, 2017 14:24

sbidoul approved these changes Apr 20, 2017

View reviewed changes

ThomasBinsfeld approved these changes Apr 20, 2017

View reviewed changes

grindtildeath approved these changes Apr 20, 2017

View reviewed changes

max3903 merged commit 17facb4 into OCA:10.0 Apr 20, 2017

Mig mgmtsystem action to v. 10.0 #168

Mig mgmtsystem action to v. 10.0 #168

Conversation

eugen-don commented Mar 14, 2017 • edited by sbidoul Loading

sbidoul commented Mar 27, 2017

eugen-don commented Mar 27, 2017

eugen-don commented Mar 27, 2017 • edited Loading

eugen-don commented Mar 27, 2017

grindtildeath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugen-don Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugen-don Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugen-don Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul commented Mar 28, 2017

eugen-don commented Mar 29, 2017

sbidoul commented Mar 29, 2017

grindtildeath commented Mar 30, 2017

max3903 left a comment

Choose a reason for hiding this comment

eugen-don commented Apr 20, 2017 • edited Loading

sbidoul commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grindtildeath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugen-don Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

sbidoul commented Apr 20, 2017

eugen-don commented Apr 20, 2017 • edited Loading

eugen-don commented Mar 14, 2017 •

edited by sbidoul

Loading

eugen-don commented Mar 27, 2017 •

edited

Loading

eugen-don Mar 29, 2017 •

edited

Loading

eugen-don Mar 29, 2017 •

edited

Loading

eugen-don Mar 29, 2017 •

edited

Loading

eugen-don commented Apr 20, 2017 •

edited

Loading

eugen-don Apr 20, 2017 •

edited

Loading

eugen-don commented Apr 20, 2017 •

edited

Loading