Better partner_firstname. #104

yajo · 2015-03-30T08:18:12Z

Pros:

Cleaner v8 API code.
Cleaner tests.
Less overload of framework methods.
Instead of having a required last name field, now it's required to
provide either the last or the first name.

Cons:

Broken backwards compatibility when copying a partner. The "(copy)"
suffix is added to the first name instead of the last name. However,
the user is expected to replace that word for the real one, so this
should not be a big problem.
Translations must be updated.

oca-clabot · 2015-03-30T08:18:14Z

Hey @yajo, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

@yajo (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

coveralls · 2015-03-30T10:37:50Z

Coverage decreased (-2.49%) to 47.93% when pulling 1203f38 on grupoesoc:better_partner_firstname into 856babf on OCA:8.0.

yajo · 2015-03-30T11:37:48Z

Sorry because I sent the pull request and later I saw some mistakes. Now it's ready for review. I already sent the signed CLA.

The point in upgrading this module is that I'm also creating a partner_lastname2 module that I need and I think it can be useful for other users of this repo. I'll post it too ASAP.

gurneyalex · 2015-03-30T14:57:42Z

@yajo could you tell me under which name the CLA has been sent? I'm not able to identify you from the github login only. Thanks

yvaucher · 2015-04-08T14:50:30Z

partner_firstname/__openerp__.py

    'description': """
-This module splits first name and last name for non company partners
-====================================================================
+Split first name and last name for non company partners


Can you move description under README.rst ? using OCA template: https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst

coveralls · 2015-04-10T08:42:14Z

Coverage decreased (-2.6%) to 47.82% when pulling 4e728c9 on grupoesoc:better_partner_firstname into 856babf on OCA:8.0.

yajo · 2015-04-20T11:02:51Z

What do the coverage messages mean? Is there anything left to be done? Thanks.

pedrobaeza · 2015-04-20T11:05:15Z

It means the quantity of the repo code that it's been tested automatically. The ideal is 100%.

More reviews are needed to be merged.

coveralls · 2015-05-15T12:12:29Z

Coverage decreased (-2.21%) to 49.26% when pulling 18650fb on grupoesoc:better_partner_firstname into 6628900 on OCA:8.0.

bwrsandman · 2015-05-15T13:43:49Z

partner_firstname/exceptions.py

+
+    @property
+    def value(self):
+        raise NotImplementedError()


What is the point of subclassing if it is only used once? Can we merge these two Exceptions?

It's a common pattern I follow for modules: do a base exception for that module and subclass it. It helps with submodules and tests. However, seems like after finishing only one possible exception was left, so I will merge it as you say.

bwrsandman · 2015-05-15T13:45:46Z

Could we add a test for _install_firstname and for the exceptions' value functions?

There are also a few branches in _name_inverse which could use a test case.

bwrsandman · 2015-05-15T13:47:39Z

👍
Overall, this looks like a very good improvement.
Great job!

yajo · 2015-05-18T07:51:08Z

Could we add a test for _install_firstname

Hardly, the method's effect can be checked only by creating a partner, then installing the module and then checking the partner's firstname and lastname, because after installing the module all that happens automatically with the constraints.

On the other hand, to run tests, you have to install the module first.

Suggestions are welcome, however.

and for the exceptions' value functions?

After the last update, it's not necessary now.

There are also a few branches in _name_inverse which could use a test case.

I'm working on all of it, I'll update soon. Thanks everyone!

yajo · 2015-05-20T10:51:48Z

Everything fixed AFAIK.

nbessi · 2015-05-20T11:28:49Z

partner_firstname/models.py

+    @api.depends("firstname", "lastname")
+    def _name_compute(self):
+        """Write the 'name' field according to splitted data."""
+        self.name = " ".join((p for p in (self.lastname,


Just a detail but u" ".join seems better

nbessi · 2015-05-20T11:35:25Z

Some details but it is globally OK.

Thanks

antespi · 2015-06-30T12:28:58Z

👍 tested on production instance

antespi · 2015-06-30T16:12:12Z

@yajo I've PR https://github.com/grupoesoc/partner-contact/pull/1 to you to be able to select names order ('firstname lastname' or 'lastname firstname'), please review, thanks

yajo · 2015-07-01T08:10:11Z

@antespi Wow you were faster than me in testing on production! 😄

To others... seems like runbots hate me 😕:

Travis says: No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.. My bug or travis'?
Coveralls says: coverage decreased (-0.91%) to 47.65%, but then changed files: 0. 😖 My bug or coveralls'?

rafaelbn · 2015-07-01T08:32:07Z

@yajo we have in several production enviroment firstname's module with a couple of issues that are correctect with your fix, that's why! :-D
👍

This happened because the invert method was not being called when using the UI, and because lastname & firstname fields were required while hidden.

yajo · 2015-07-01T11:45:15Z

@antespi @rafaelbn Glad to hear that. If so, please update to last version, it fixes some critical usability bugs I found today.

yajo · 2015-07-03T10:22:22Z

Coveralls fails because the code is shorter, so it should be considered OK.

pedrobaeza · 2015-07-03T11:22:47Z

👍

Better partner_firstname.

yajo · 2015-07-03T12:06:38Z

Sorry to bother you again, but I found a bug with the onchange method that I'm fixing right now. Maybe you want to roll this back before it's too late.

pedrobaeza · 2015-07-03T12:10:20Z

Please make it the PR fast and I'll fast track it.

yajo · 2015-07-04T13:14:11Z

I could not fix it in time, I'll patch it this monday. Sorry for that, the bug was just in the very one line that is not tested 😁. Is there a way to unit test onchange methods?

hbrunn · 2015-07-04T13:25:22Z

@yajo say

with self.env.do_in_onchange():
    self.env['my.model'].new(whatever_values).my_onchange_method()

yajo · 2015-07-05T07:18:41Z

@hbrunn You guys are awesome. You'll have the fix tomorrow.

oca-clabot · 2016-10-24T13:06:24Z

Hey @yajo,
We acknowledge that the following users have signed our Contributor License Agreement:

@yajo

Appreciation of efforts,
OCA CLAbot

yajo mentioned this pull request Mar 30, 2015

Partner lastname2 #105

Closed

yvaucher reviewed Apr 8, 2015
View reviewed changes

bwrsandman reviewed May 15, 2015
View reviewed changes

gurneyalex changed the title ~~Beter partner_firstname.~~ Better partner_firstname. May 17, 2015

bwrsandman added the needs review label May 20, 2015

nbessi reviewed May 20, 2015
View reviewed changes

Jairo Llopis added 4 commits June 29, 2015 09:49

Credits to last translator.

40af032

Increase coverage.

ac719b6

Add args to exception to display the correct message in the UI.

2dcb159

Execute _firstname_install when installing, and log it.

4b9ab2d

Jairo Llopis added 4 commits July 1, 2015 10:47

Rename methods to follow guidelines.

299b4e2

Better docstrings.

79e37f8

Workaround odoo/odoo#6324.

87171bb

Fix users not being able to create companies.

2494815

This happened because the invert method was not being called when using the UI, and because lastname & firstname fields were required while hidden.

Add new tests and fix the resulting bugs.

6ffca48

This was referenced Jul 2, 2015

Coveralls PR check bot broken #136

Closed

Coveralls PR check bot broken OCA/maintainer-quality-tools#208

Closed

pedrobaeza added a commit that referenced this pull request Jul 3, 2015

Merge pull request #104 from grupoesoc/better_partner_firstname

1520e0d

Better partner_firstname.

pedrobaeza merged commit 1520e0d into OCA:8.0 Jul 3, 2015

yvaucher removed the needs review label Jul 3, 2015

antespi mentioned this pull request Jul 3, 2015

Update "display name" fields when updating first / last names. #138

Closed

yajo mentioned this pull request Jul 6, 2015

[8.0] partner_firstname: Recursive @onchange misbehavior #139

Merged

antespi mentioned this pull request Jul 17, 2015

[FIX] Recursive onchange bug #143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better partner_firstname. #104

Better partner_firstname. #104

yajo commented Mar 30, 2015

oca-clabot commented Mar 30, 2015

coveralls commented Mar 30, 2015

yajo commented Mar 30, 2015

gurneyalex commented Mar 30, 2015

yvaucher Apr 8, 2015

coveralls commented Apr 10, 2015

yajo commented Apr 20, 2015

pedrobaeza commented Apr 20, 2015

coveralls commented May 15, 2015

bwrsandman May 15, 2015

yajo May 18, 2015

bwrsandman commented May 15, 2015

bwrsandman commented May 15, 2015

yajo commented May 18, 2015

yajo commented May 20, 2015

nbessi May 20, 2015

nbessi commented May 20, 2015

antespi commented Jun 30, 2015

antespi commented Jun 30, 2015

yajo commented Jul 1, 2015

rafaelbn commented Jul 1, 2015

yajo commented Jul 1, 2015

yajo commented Jul 3, 2015

pedrobaeza commented Jul 3, 2015

yajo commented Jul 3, 2015

pedrobaeza commented Jul 3, 2015

yajo commented Jul 4, 2015

hbrunn commented Jul 4, 2015

yajo commented Jul 5, 2015

oca-clabot commented Oct 24, 2016

Better partner_firstname. #104

Better partner_firstname. #104

Conversation

yajo commented Mar 30, 2015

oca-clabot commented Mar 30, 2015

coveralls commented Mar 30, 2015

yajo commented Mar 30, 2015

gurneyalex commented Mar 30, 2015

yvaucher Apr 8, 2015

Choose a reason for hiding this comment

coveralls commented Apr 10, 2015

yajo commented Apr 20, 2015

pedrobaeza commented Apr 20, 2015

coveralls commented May 15, 2015

bwrsandman May 15, 2015

Choose a reason for hiding this comment

yajo May 18, 2015

Choose a reason for hiding this comment

bwrsandman commented May 15, 2015

bwrsandman commented May 15, 2015

yajo commented May 18, 2015

yajo commented May 20, 2015

nbessi May 20, 2015

Choose a reason for hiding this comment

nbessi commented May 20, 2015

antespi commented Jun 30, 2015

antespi commented Jun 30, 2015

yajo commented Jul 1, 2015

rafaelbn commented Jul 1, 2015

yajo commented Jul 1, 2015

yajo commented Jul 3, 2015

pedrobaeza commented Jul 3, 2015

yajo commented Jul 3, 2015

pedrobaeza commented Jul 3, 2015

yajo commented Jul 4, 2015

hbrunn commented Jul 4, 2015

yajo commented Jul 5, 2015

oca-clabot commented Oct 24, 2016