[8.0] New module: phonecall_plan #94

Moret84 · 2016-05-17T09:15:41Z

This module provides a wizard to plan calls from partners/leads views.

coveralls · 2016-05-17T09:22:45Z

Coverage decreased (-0.9%) to 78.07% when pulling 7773fa2 on Moret84:8.0 into 9f830ec on OCA:8.0.

elicoidal

Thanks for the contribution!

elicoidal · 2016-10-09T07:20:12Z

phonecall_plan/README.rst

@@ -0,0 +1,70 @@
+.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg


@Moret84 Thanks for your contribution to the OCA.
Let me introduce to you some basic information about contribution in OCA and I will review other points in more comments inside the code..
In general, please follow the principles:

Contribution guidelines: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md

Please follow the OCA README template:
https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst

We recommend to include a screenshot as well to help new comers (nice to have)

Please use simple headers in all py files following OCA templates
https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L3

elicoidal · 2016-10-09T07:20:18Z

phonecall_plan/README.rst

+      :alt: License: AGPL-3
+
+==============
+phonecall_plan


Use plain English "Phonecall Plan"

elicoidal · 2016-10-09T07:21:04Z

phonecall_plan/README.rst

+phonecall_plan
+==============
+
+This module add a wizard to allow you to plan call from lead/partner view.


"This module adds a wizard to allow you to plan calls from the lead and partner views."

elicoidal · 2016-10-09T07:21:35Z

phonecall_plan/README.rst

+
+This module add a wizard to allow you to plan call from lead/partner view.
+
+Installation


Remove the section which does not add any added value (this is the standard process)

elicoidal · 2016-10-09T07:21:44Z

phonecall_plan/README.rst

+
+To install this module, you just need to select it from availables modules
+
+Configuration


elicoidal · 2016-10-09T07:24:01Z

phonecall_plan/__init__.py

@@ -0,0 +1,22 @@
+# -*- coding: utf-8 -*-


please use short headers

elicoidal · 2016-10-09T07:25:13Z

phonecall_plan/__openerp__.py

+    'name': 'Phonecall plan',
+    'summary': "Wizard to plan calls from leads/partners",
+    'version': '8.0.1.0.0',
+    'depends': ['base', 'crm'],


Nothing wrong here but you can improve it putting the dependencies and data files in a separate line (check other OCA modules)
Non-blocking though

elicoidal · 2016-10-09T07:25:35Z

phonecall_plan/i18n/phonecall_plan.pot

@@ -0,0 +1,99 @@
+# Translation of Odoo Server.


remove this file

elicoidal · 2016-10-09T07:25:48Z

phonecall_plan/wizards/__init__.py

@@ -0,0 +1,22 @@
+# -*- coding: utf-8 -*-


headers (all py files)

elicoidal · 2016-10-09T07:27:06Z

phonecall_plan/wizards/phonecall_plan_view.xml

+			<field name="name">Scheduled calls</field>
+			<field name="model">add.call</field>
+			<field name="arch" type="xml">
+				<form>


Can you improve the indentation (too wide/too many spaces per indentation)?

I have 4 spaces per indent. Do you think it is too much? Should I use 2 instead ?

I think you have tabs instead of spaces

Moret84 · 2016-10-09T22:10:02Z

Thanks for your review. So sorry, I contribute for the very first time. I read carefully all docs, but it seems I forgot some parts. I've made a commit with all changes from your comments.

pedrobaeza · 2016-10-10T01:34:28Z

phonecall_plan/README.rst

+
+To use this module, you need to:
+
+More


Why this title?

Do you have an other idea for the title ?

No, just remove it.

pedrobaeza · 2016-10-10T01:35:06Z

phonecall_plan/__init__.py

@@ -0,0 +1,22 @@
+# coding: utf-8
+##############################################################################


Short headers according template https://github.com/OCA/maintainer-tools/blob/master/template/module/__init__.py

pedrobaeza · 2016-10-10T01:36:47Z

phonecall_plan/__init__.py

+##############################################################################
+#
+#    Odoo, Open Source Management Solution
+#    Copyright (C) 2015 Fogits Solutions - http://www.fogits.com


Why is here another company name?

pedrobaeza · 2016-10-10T01:36:50Z

phonecall_plan/__openerp__.py

+        'base',
+        'crm'
+    ],
+    'author': 'Aurélien Rivet, 6IT, Odoo Community Association (OCA)',


Here there should be only companies, nor individuals (unless you are on your own). I see 2 author entries here.

pedrobaeza · 2016-10-10T01:37:21Z

phonecall_plan/wizards/phonecall_plan.py

+        return {}
+
+
+class AddPartnerCall(models.TransientModel):


One model per file.

pedrobaeza · 2016-10-10T01:38:03Z

phonecall_plan/wizards/phonecall_plan.py

+        obj_phone = self.env['crm.phonecall']
+        for partner in self.partner_ids:
+            obj_phone.create(
+                {


You can save one line and one indentation level if you put this bracket in the previous line.

pedrobaeza · 2016-10-10T01:38:16Z

phonecall_plan/wizards/phonecall_plan.py

+                    'user_id':  self.salesperson_id.id,
+                    'state': "open",
+                })
+        return {}


Just return True.

pedrobaeza · 2016-10-10T01:38:36Z

Can you include tests?

pedrobaeza · 2016-10-20T00:41:04Z

@Moret84, are you going to work on the comments?

Moret84 · 2016-10-20T05:37:57Z

Yeah I'm sorry . I changed my code. But I haven't written test yet. I am afraid I don't really know how to write effective of them. I feel a bit ashamed.

pedrobaeza · 2016-10-20T06:25:59Z

OK, no problem for the tests. They are not mandatory.

Moret84 · 2016-10-21T23:38:11Z

Oops I forgot one point of your review in my latest commit (one model per file). I also noticed a check didn't pass because of the deprecated "active" key in the manifest file. Will fix it soon.

rafaelbn · 2017-05-11T12:54:15Z

Hi @Moret84 , thanks for this contribution. This module looks very useful an if you could continue with it we will help you in reviews. Let us know please. Thanks again

rafaelbn · 2017-05-11T12:55:11Z

Please, rename module to crm_phonecall_plan

misern2

Code Review

misern2 · 2017-05-30T11:23:44Z

crm_phonecall_plan/README.rst

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


The Runbot link does not work.

Hi @misern2. I have seen that the link points to the top-menu entry: website. If you are entering for first time you can change the url with http://3293144-94-bbe389.runbot2.odoo-community.org/web to enter the login page.I hope this helps.

rafaelbn · 2017-05-31T15:23:58Z

Please @elicoidal @pedrobaeza unblock this PR, thanks!

JoanDx · 2017-05-31T15:51:26Z

When i try to test it from partner view, when i try to confirm the wizard it show an error like:
`Integrity Error

The operation cannot be completed, probably due to the following:

deletion: you may be trying to delete a record while other records still reference it
creation/update: a mandatory field is not correctly set

[object with reference: summary_id - summary.id] `
And from Lead view it doesnt appear nothing like plan a call in submenu More.

I hope it helps.

elicoidal

some details. No test
please attend comment on error

elicoidal · 2017-06-01T00:36:04Z

crm_phonecall_plan/README.rst

+
+==============
+CRM Phonecall Plan
+==============


Lines must be at least as long as the title

elicoidal · 2017-06-01T00:37:23Z

crm_phonecall_plan/wizards/add_partner_call.py

+        for partner in self.partner_ids:
+            obj_phone.create({
+                'date': self.date,
+                'name': _('To fill !!!'),


Please improve description...

rafaelbn · 2017-06-08T08:24:38Z

Hi @Moret84 , please could you review comments? Are you going to continue with this PR?

Moret84 · 2017-06-08T08:39:50Z

Hi @rafaelbn I am not working on Odoo anymore so I won't continue with this PR.

New module: phonecaall_plan.

7773fa2

This module provides a wizard to plan calls from partners/leads views.

Moret84 changed the title ~~New module: phonecaall_plan.~~ New module: phonecall_plan May 17, 2016

Moret84 changed the title ~~New module: phonecall_plan~~ [8.0] New module: phonecall_plan May 23, 2016

elicoidal suggested changes Oct 9, 2016

View reviewed changes

Improvements to module format based on comments.

e8a7a04

pedrobaeza requested changes Oct 10, 2016

View reviewed changes

Indentation, headers and README fixes to be more OCA-compliant.

9987905

Moret84 added 2 commits October 22, 2016 13:27

One model per file. Remove deprecated key "active" in manifest file.

785151c

Code improvements to match PEP8.

e981775

rafaelbn added this to the 8.0 milestone May 11, 2017

rafaelbn added the needs review label May 11, 2017

Change module name to crm_phonecall_plan

bbe3894

misern2 reviewed May 30, 2017

View reviewed changes

elicoidal suggested changes Jun 1, 2017

View reviewed changes

yajo added the needs fixing label Jun 1, 2017

Moret84 closed this Jun 8, 2017

rafaelbn reopened this Jul 10, 2017

rafaelbn closed this Jul 11, 2017

rafaelbn reopened this Jul 11, 2017

rafaelbn closed this Jul 24, 2017

		@@ -0,0 +1,70 @@
		.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg


		This module add a wizard to allow you to plan call from lead/partner view.

		Installation


		To install this module, you just need to select it from availables modules

		Configuration

		@@ -0,0 +1,22 @@
		# coding: utf-8
		##############################################################################

[8.0] New module: phonecall_plan #94

[8.0] New module: phonecall_plan #94

Conversation

Moret84 commented May 17, 2016

coveralls commented May 17, 2016 • edited Loading

elicoidal left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moret84 commented Oct 9, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 10, 2016

pedrobaeza commented Oct 20, 2016

Moret84 commented Oct 20, 2016

pedrobaeza commented Oct 20, 2016

Moret84 commented Oct 21, 2016

rafaelbn commented May 11, 2017

rafaelbn commented May 11, 2017

misern2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented May 31, 2017

JoanDx commented May 31, 2017

elicoidal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Jun 8, 2017

Moret84 commented Jun 8, 2017

coveralls commented May 17, 2016 •

edited

Loading