[REF] 8.0 mgmtsystem_action #22

llacroix · 2014-11-20T17:03:26Z

The current module inherit crm.claim while the state attribute doesn't exist anymore. I made some changes to use the stage_id instead but I'm not so sure it's a good idea. I'm open to suggestions.

coveralls · 2014-11-20T21:58:27Z

Coverage remained the same when pulling ea2e765 on savoirfairelinux:8.0-mgmtsystem_action into ad23dae on OCA:8.0.

dreispt · 2014-11-20T22:03:44Z

mgmtsystem_action/__openerp__.py

@@ -43,6 +43,6 @@
        'board_mgmtsystem_action.xml',
    ],
    "demo": ['demo_action.xml'],
-    'installable': False,
+    'installable': True,
 }
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:


Not a blocker, but it would be nice if you removed the vim lines.

bwrsandman · 2014-11-21T15:20:14Z

mgmtsystem_action/board_mgmtsystem_action.xml

            <field name="res_model">mgmtsystem.action</field>
            <field name="view_type">form</field>
            <field name="view_mode">tree</field>
            <field name="view_id" ref="view_mgmtsystem_action_tree"/>
-            <field name="domain">[('state', 'in', ('draft','open','pending'))]</field>
+	    <!-- TODO check that names match what's present -->


Is this a work in progress?

No, but was looking for some pointers.

coveralls · 2014-11-21T18:05:29Z

Coverage decreased (-40.38%) when pulling 2d7f622 on savoirfairelinux:8.0-mgmtsystem_action into d10997c on OCA:8.0.

llacroix · 2014-11-22T13:01:57Z

Can someone have a look why travis crashes?

dreispt · 2014-11-22T22:31:59Z

Restarted it and it doesn't solve the issue.
It seems to be in the report module.
Could it be related to odoo/odoo#3811 ?

pedrobaeza · 2014-11-23T09:13:28Z

Yeah, I'm experimenting same issue on l10n-spain branch, so it's related to odoo core. For now, I haven't investigated any solution.

coveralls · 2014-11-24T09:24:10Z

Coverage decreased (-7.69%) when pulling 0546659 on savoirfairelinux:8.0-mgmtsystem_action into d10997c on OCA:8.0.

llacroix · 2014-11-24T09:24:43Z

Good news. Seems to work now

dreispt · 2014-11-24T09:48:29Z

The state/stage stuff doesn't bother me a lot, since I believe that should be revisited at a later moment. So IMO the commented stuff on it can perfectly be left in the code for now.
As long as it's working, you have my 👍

bwrsandman · 2014-11-24T13:42:24Z

Coverage has gone down -7.69%, please add tests for mgmtsystem_action/mgmtsystem_action.py

create
message_auto_subscribe
case_open
case_close
get_action_url

dreispt · 2014-11-24T13:57:43Z

Come on @bwrsandman: it's not like it's a new module. It's just a port from v7.
We can only demand that it's working on v8 and follows the style guide.

bwrsandman · 2014-11-24T14:01:36Z

This is a strong recommendation, I won't block it, but I won't approve it without tests.

@dreispt I have worked on porting the mgmsystem modules to v7 when I started and I know how much the code needs be better. Tests are the most beneficial when porting to a new version since behaviour is much more likely to do unintended things.

A drop in coverage (esp. from 100%) should be objected.

dreispt · 2014-11-24T14:08:42Z

I see, you're asking it but not demanding - that's OK.
I would ague the right comparison would be with v7's 46%.

From my PoV I would like to see the just "working" in v8 ASAP so that we have the base to improve it. Much of the current code is related to state/stage management, and that will probably be retired.
That's why I don't mind a lot about the missing tests now.

It is used and only contains things from mgmtsystem_nonconformity

llacroix · 2014-11-25T12:45:22Z

Should I remove the workflow file, it doesn't seem to be required anymore? I'm not sure it is still relevant.

management-system/mgmtsystem_action/workflow_mgmtsystem_action.xml

Line 60 in 9f8f27e

coveralls · 2014-11-25T12:58:46Z

Coverage decreased (-2.33%) when pulling 6559279 on savoirfairelinux:8.0-mgmtsystem_action into d10997c on OCA:8.0.

When doing `case.user_id = None`, it will throw this error: IntegrityError: null value in column "partner_id" violates not-null constraint

dreispt · 2014-11-25T13:08:26Z

👏

coveralls · 2014-11-25T13:11:51Z

Coverage remained the same when pulling 304e365 on savoirfairelinux:8.0-mgmtsystem_action into d10997c on OCA:8.0.

pedrobaeza · 2014-11-25T13:17:26Z

Excellent coverage!

You can remove openupgrade analysis file and migration scripts, because they are for v7. You should build another ones for v8 if there are changes in DB layout.

bwrsandman · 2014-11-25T14:09:30Z

👍
~~You could add a test case with no user_id to get 100% coverage if you want.~~

As for the transition, I still think it's in use. What makes you think it should be removed?

max3903 · 2014-11-25T14:14:05Z

Workflow is not relevant if people involved in the action are added as followers and receive notifications of state changes.

pedrobaeza · 2014-11-25T14:21:51Z

@bwrsandman, are you asking me? I ask about removing migration files because it's for v6 to v7 migration, not for v7 to v8.

bwrsandman · 2014-11-25T14:23:40Z

@pedrobaeza Sorry for the confusion: I am asking @llacroix

Should I remove the workflow file, it doesn't seem to be required anymore? I'm not sure it is still relevant.

llacroix · 2014-11-25T15:06:45Z

@bwrsandman The workflow transition was calling case_close which only do things related to mgmtsystem_nonconformity In that case, the workflow for transition should be moved to the mgmtsystem_nonconformity module.

The other workflow for case_open seems to send email and the url in the email is http://localhost:8080. Unless it gets replaced automagically somewhere else the url is wrong.

llacroix · 2014-11-25T15:08:32Z

@pedrobaeza how about updating from 6 to 8 ? Is it possible or the "safe" way would be to update from 6 to 7 and from 7 to 8.

Right now, there is no migration from 7 to 8 the model didn't change as far as I can tell.

bwrsandman · 2014-11-25T15:17:29Z

@llacroix good point, that should be moved to NC, then,

Unless it gets replaced automagically somewhere else the url is wrong.

The url will get changed depending on your deployment. Try changing your port, you'll see it changes.

how about updating from 6 to 8 ?

You can only do an upgrade from one version to the next:
6.0 -> 8.0 is actually 6.0 -> 6.1 -> 7.0 -> 8.0

Maybe in the future OpenUpgrade could support such a high jump but it is not the case today.

That being said, I don't really mind if the old migrations stay, they serve as examples for future migration scripts and they shouldn't cause any problems for people in v8

pedrobaeza · 2014-11-25T15:50:11Z

OK, we have those scripts on v7 branch, but if you want to keep it, it's not a blocking issue.

coveralls · 2014-11-25T18:20:48Z

Coverage remained the same when pulling b5f3e37 on savoirfairelinux:8.0-mgmtsystem_action into d10997c on OCA:8.0.

llacroix · 2014-11-26T16:32:17Z

Is there anything else to do so we can pass to other modules? Many modules can't be merged because they depend on this one.

dreispt · 2014-11-26T18:23:54Z

@llacroix To merge this ASAP.
AFAICT there are no blocking issues, right?

bwrsandman · 2014-11-26T22:03:45Z

👍 No huge issue (in fact it looks amazing), but I would wait for @max3903 to approve

max3903 · 2014-11-28T14:18:16Z

👍

[REF] 8.0 mgmtsystem_action

[ADD] document_url: Module to attach URLs as documents

BSCOS-100 Language and tags on company's contacts form

llacroix changed the title ~~8.0 mgmtsystem action~~ [REF] mgmtsystem_action Nov 20, 2014

dreispt reviewed Nov 20, 2014
View reviewed changes

llacroix added 5 commits November 21, 2014 18:10

Ported mgmtsystem_action

c84110a

Fix typo and pep8

70644fc

Changes made for API v8

f2ac6f0

Removed pdb lines

8082335

Removed vim lines and fixed domain

67f1498

llacroix force-pushed the 8.0-mgmtsystem_action branch from ea2e765 to 67f1498 Compare November 21, 2014 15:12

llacroix changed the title ~~[REF] mgmtsystem_action~~ [REF] 8.0 mgmtsystem_action Nov 21, 2014

bwrsandman reviewed Nov 21, 2014
View reviewed changes

Removed comments and commented code

2d7f622

llacroix added 2 commits November 22, 2014 15:50

Added tests and updated code to v8

45fbf8a

Fix flake8 errors

0546659

dreispt mentioned this pull request Nov 23, 2014

Pylint failing with "Message symbol 'print-statement' is already defined" OCA/maintainer-quality-tools#143

Closed

dreispt mentioned this pull request Nov 24, 2014

[REF] 8.0 mgmtsystem claim #29

Merged

1 task

llacroix added 2 commits November 25, 2014 15:36

Updated according to reviews

e0b4a46

Removed case_close

9f8f27e

It is used and only contains things from mgmtsystem_nonconformity

Removed blank line, flake8 not happy

6559279

Removed check for user_id as user_id cannot be null

304e365

When doing `case.user_id = None`, it will throw this error: IntegrityError: null value in column "partner_id" violates not-null constraint

Remove transition case_close and moved icon

b5f3e37

bwrsandman added a commit that referenced this pull request Nov 28, 2014

Merge pull request #22 from savoirfairelinux/8.0-mgmtsystem_action

ab18896

[REF] 8.0 mgmtsystem_action

bwrsandman merged commit ab18896 into OCA:8.0 Nov 28, 2014

yvaucher removed the needs review label Nov 28, 2014

llacroix referenced this pull request in savoirfairelinux/management-system Dec 29, 2014

Merge pull request #22 from pedrobaeza/8.0-document_url

7162d7b

[ADD] document_url: Module to attach URLs as documents

llacroix mentioned this pull request Apr 1, 2015

[REF] 8.0 mgmtsystem nonconformity #25

Merged

4 tasks

max3903 mentioned this pull request Apr 7, 2015

Migration to version 8.0 #49

Closed

26 tasks

tschanzt pushed a commit to camptocamp/management-system that referenced this pull request Jun 26, 2019

Merge pull request OCA#22 from Timo17100-c2c/bscos-100

4f19fc8

BSCOS-100 Language and tags on company's contacts form

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] 8.0 mgmtsystem_action #22

[REF] 8.0 mgmtsystem_action #22

llacroix commented Nov 20, 2014

coveralls commented Nov 20, 2014

dreispt Nov 20, 2014

bwrsandman Nov 21, 2014

llacroix Nov 21, 2014

coveralls commented Nov 21, 2014

llacroix commented Nov 22, 2014

dreispt commented Nov 22, 2014

pedrobaeza commented Nov 23, 2014

coveralls commented Nov 24, 2014

llacroix commented Nov 24, 2014

dreispt commented Nov 24, 2014

bwrsandman commented Nov 24, 2014

dreispt commented Nov 24, 2014

bwrsandman commented Nov 24, 2014

dreispt commented Nov 24, 2014

llacroix commented Nov 25, 2014

coveralls commented Nov 25, 2014

dreispt commented Nov 25, 2014

coveralls commented Nov 25, 2014

pedrobaeza commented Nov 25, 2014

bwrsandman commented Nov 25, 2014

max3903 commented Nov 25, 2014

pedrobaeza commented Nov 25, 2014

bwrsandman commented Nov 25, 2014

llacroix commented Nov 25, 2014

llacroix commented Nov 25, 2014

bwrsandman commented Nov 25, 2014

pedrobaeza commented Nov 25, 2014

coveralls commented Nov 25, 2014

llacroix commented Nov 26, 2014

dreispt commented Nov 26, 2014

bwrsandman commented Nov 26, 2014

max3903 commented Nov 28, 2014

[REF] 8.0 mgmtsystem_action #22

[REF] 8.0 mgmtsystem_action #22

Conversation

llacroix commented Nov 20, 2014

coveralls commented Nov 20, 2014

dreispt Nov 20, 2014

Choose a reason for hiding this comment

bwrsandman Nov 21, 2014

Choose a reason for hiding this comment

llacroix Nov 21, 2014

Choose a reason for hiding this comment

coveralls commented Nov 21, 2014

llacroix commented Nov 22, 2014

dreispt commented Nov 22, 2014

pedrobaeza commented Nov 23, 2014

coveralls commented Nov 24, 2014

llacroix commented Nov 24, 2014

dreispt commented Nov 24, 2014

bwrsandman commented Nov 24, 2014

dreispt commented Nov 24, 2014

bwrsandman commented Nov 24, 2014

dreispt commented Nov 24, 2014

llacroix commented Nov 25, 2014

coveralls commented Nov 25, 2014

dreispt commented Nov 25, 2014

coveralls commented Nov 25, 2014

pedrobaeza commented Nov 25, 2014

bwrsandman commented Nov 25, 2014

max3903 commented Nov 25, 2014

pedrobaeza commented Nov 25, 2014

bwrsandman commented Nov 25, 2014

llacroix commented Nov 25, 2014

llacroix commented Nov 25, 2014

bwrsandman commented Nov 25, 2014

pedrobaeza commented Nov 25, 2014

coveralls commented Nov 25, 2014

llacroix commented Nov 26, 2014

dreispt commented Nov 26, 2014

bwrsandman commented Nov 26, 2014

max3903 commented Nov 28, 2014