[12.0][MIG] web_widget_digitized_signature #1325

max3903 · 2019-07-05T17:28:40Z

#1048
Superseeds #1318

pedrobaeza · 2019-07-05T17:43:16Z

Is the other PR from a member of your team? If not, it sound irespectful to superseed without asking first.

max3903 · 2019-07-05T17:46:28Z

@pedrobaeza Yes. He pushed to his own account, instead of our organization.

pedrobaeza · 2019-07-05T17:47:02Z

OK then

pedrobaeza

Adding a lot of noise, lobbying and no valuable changes: the module would work in 12.0 as is.

web_widget_digitized_signature/__manifest__.py

pedrobaeza · 2019-07-05T18:00:38Z

web_widget_digitized_signature/__manifest__.py

    'website': 'https://github.com/OCA/web',
-    "license": "AGPL-3",
-    "category": 'Web',
+    'license': 'AGPL-3',


Don't increase the diff if not needed. Disable automatic formatting like black for this.

@pedrobaeza I think migration is a great opportunity to clean up those inconsistencies of having simple and double quote within the same manifest file.

Well, I don't see them as inconsistencies. Both are allowed in Python the same and it doesn't produce any visual problem. We don't have any guideline that forces to unify them. Even if so, that must be done on a separate commit [REF] style or similar.

web_widget_digitized_signature/__manifest__.py

pedrobaeza · 2019-07-05T18:01:22Z

web_widget_digitized_signature/models/res_users.py

-        string='Signature',
-        oldname="signature_image",
-    )
+    digital_signature = fields.Binary(string='Digital Signature',


The same about automatic formatting

You can add attachment=True and add migration script for saving space in DB

pedrobaeza · 2019-07-05T18:02:26Z

web_widget_digitized_signature/readme/CONTRIBUTORS.rst

@@ -1,4 +1,6 @@
 * Jay Vora <[email protected]>
-* Tecnativa <https://www.tecnativa.com>:


Please don't plain our formatting system for putting the company URL.

pedrobaeza · 2019-07-05T18:02:56Z

web_widget_digitized_signature/readme/CONTRIBUTORS.rst

+* Pedro M. Baeza <[email protected]>
+* Vicent Cubells <[email protected]>
+* Mayank Gosai <[email protected]>
+* Maxime Chambreuil <[email protected]>


2 contributors for this minimum changes?

web_widget_digitized_signature/static/src/js/digital_sign.js

web_widget_digitized_signature/views/res_users_view.xml

web_widget_digitized_signature/views/web_digital_sign_view.xml

max3903 · 2019-07-10T15:24:14Z

@mgosai Can you take care of the comments please to get this merged? Thanks.

pedrobaeza · 2019-07-10T16:14:36Z

Please squash all these commits

nikul-serpentcs · 2019-07-13T04:04:19Z

@max3903 @mgosai Can you please Travis

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: web-11.0/web-11.0-web_widget_digitized_signature Translate-URL: https://translation.odoo-community.org/projects/web-11-0/web-11-0-web_widget_digitized_signature/

max3903 · 2019-07-16T15:28:35Z

@nikul-serpentcs Done!

pedrobaeza · 2019-07-16T15:33:02Z

This still has undesired auto-format changes I have already mentioned.

nikul-serpentcs

Only Code Review LGTM 👍

@max3903 Please check/modify others reviewer comment's also

max3903 · 2019-07-17T13:09:46Z

@pedrobaeza All the changes are desired.

max3903 · 2019-07-17T13:09:56Z

/ocabot merge

OCA-git-bot · 2019-07-17T13:10:10Z

This PR looks fantastic, let's merge it!
Rebased to 12.0-ocabot-merge-pr-1325-by-max3903-bump-no, awaiting test results.

OCA-git-bot · 2019-07-17T13:32:49Z

Congratulations, your PR was merged at 0be0575. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 12.0.

Signed-off-by max3903

pedrobaeza · 2019-07-17T13:36:28Z

No, they aren't desired, and if you want to do it, you must split that changes in a separate commit apart from the migration as stated.

Please don't disobey next time as you always do and launch the merge command without agreement.

rafaelbn · 2019-07-18T13:19:05Z

Hi @max3903 ,

I don't think is good practice to merge with a disagree of a PCS, you should then ask maybe another PSC in @OCA/web-maintainers but not merge.

You add Coyright, Author and 2 contribution by changin " by ' 7008038 plus 4 lines of code 7008038#diff-7a2fc2224315edb7571db98b9462f6e1

We must also maintain the same criteria for all when it comes to attributing contributions or authorizations. If you make a migration of a module with a diff of 40 lines of which code change are 4 lines, then what is the criterion. You have to be fair.

This is not something that of course takes away the dream, it is not harmful for Tecnativa. But it is harmful and unfair in my personal opinion for OCA and the rest of the contributors.

Regards

max3903 · 2019-07-18T13:33:52Z

Hi @rafaelbn,

The changes I pushed are cosmetic changes to facilitate code readibility and have consistency:

Having double quote and single quote in the same manifest doesn't make sense to me.
Having XML file with 120+ characters line either.

I don't see the point of doing separate pull requests of the same module for that and asking people to review this separately. Let's not waste everyone else time on this. Especially when we ask people later to squash commits...

Also this is a Tecnativa module that we decided to maintain, so give me a break. If you wanted to take care of it and maintain it, you had plenty of time to provide an alternative maintainer.

I think we discussed it long enough last year, this kind of behavior is negative for the community. We should welcome contributions and improve them, not reject them.

rafaelbn · 2019-07-18T13:42:44Z

We should welcome contributions and improve them, not reject them

Absolutely agree here, I'm very concerned about expanding OCA

pedrobaeza · 2019-07-18T14:19:19Z

I was asking for 2 separated commits, not 2 PRs as now it's very difficult to know which changes are due to 12.0 needs and what are only cosmetic changes. It's not too much to ask.

hbrunn · 2019-07-18T16:27:33Z

I agree it's a good thing to ask contributors to keep the history clean. So very often I'm stuck in some combined commit when I want to follow how some code came to place, I appreciate every effort to be strict there.

But given the 2 PRs / 2 commits misunderstanding, can we file it as such and move on? And let's learn from it and write some suggestions about this in CONTRIBUTING.rst.

max3903 added needs review migration labels Jul 5, 2019

max3903 added this to the 12.0 milestone Jul 5, 2019

max3903 assigned mgosai Jul 5, 2019

max3903 mentioned this pull request Jul 5, 2019

[12.0][MIG] web_widget_digitized_signature #1318

Closed

OCA-git-bot mentioned this pull request Jul 5, 2019

Migration to version 12.0 #1048

Closed

40 tasks

max3903 force-pushed the 12.0-mig-web_widget_digitized_signature branch from 3478aca to 2604908 Compare July 5, 2019 17:40

max3903 requested review from mgosai, cubells and pedrobaeza July 5, 2019 17:41

max3903 force-pushed the 12.0-mig-web_widget_digitized_signature branch from 2604908 to 200e2f8 Compare July 5, 2019 17:44

mgosai approved these changes Jul 5, 2019

View reviewed changes

pedrobaeza requested changes Jul 5, 2019

View reviewed changes

max3903 mentioned this pull request Jul 5, 2019

[12.0][MIG] sale_order_digitized_signature OCA/sale-workflow#892

Merged

1 task

cubells and others added 10 commits July 16, 2019 10:27

[ADD] web_widget_digitized_signature

34a952c

[IMP] web_widget_digitized_signature: Add tests

846cb4c

OCA Transbot updated translations from Transifex

b7ae65a

[FIX] Incorrect field name to track signature (OCA#691)

95ece59

porting web_widget_digitized_signature to 10.0

0ebe051

OCA Transbot updated translations from Transifex

c7664f8

[MIG] web_widget_digitized_signature: Migration to 11.0

96093e6

Update __manifest__.py

bfe13e8

[UPD] Update web_widget_digitized_signature.pot

b6feaf7

[UPD] README.rst

ceb2c42

oca-transbot and others added 2 commits July 16, 2019 10:27

Update translation files

9b20580

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: web-11.0/web-11.0-web_widget_digitized_signature Translate-URL: https://translation.odoo-community.org/projects/web-11-0/web-11-0-web_widget_digitized_signature/

[MIG] web_widget_digitized_signature: Migration to 12.0

7008038

max3903 force-pushed the 12.0-mig-web_widget_digitized_signature branch from 4156e3d to 7008038 Compare July 16, 2019 15:28

max3903 requested a review from nikul-serpentcs July 16, 2019 15:58

nikul-serpentcs approved these changes Jul 17, 2019

View reviewed changes

OCA-git-bot merged commit 7008038 into OCA:12.0 Jul 17, 2019

OCA-git-bot added a commit that referenced this pull request Jul 17, 2019

Merge PR #1325 into 12.0

0be0575

Signed-off-by max3903

OCA-git-bot added the merged 🎉 label Jul 17, 2019

max3903 deleted the 12.0-mig-web_widget_digitized_signature branch July 17, 2019 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0][MIG] web_widget_digitized_signature #1325

[12.0][MIG] web_widget_digitized_signature #1325

max3903 commented Jul 5, 2019

pedrobaeza commented Jul 5, 2019

max3903 commented Jul 5, 2019

pedrobaeza commented Jul 5, 2019

pedrobaeza left a comment

pedrobaeza Jul 5, 2019

max3903 Jul 10, 2019

pedrobaeza Jul 10, 2019

pedrobaeza Jul 5, 2019

pedrobaeza Jul 5, 2019

pedrobaeza Jul 5, 2019

pedrobaeza Jul 5, 2019

max3903 commented Jul 10, 2019

pedrobaeza commented Jul 10, 2019

nikul-serpentcs commented Jul 13, 2019

max3903 commented Jul 16, 2019

pedrobaeza commented Jul 16, 2019

nikul-serpentcs left a comment

max3903 commented Jul 17, 2019

max3903 commented Jul 17, 2019

OCA-git-bot commented Jul 17, 2019

OCA-git-bot commented Jul 17, 2019

pedrobaeza commented Jul 17, 2019

rafaelbn commented Jul 18, 2019

max3903 commented Jul 18, 2019

rafaelbn commented Jul 18, 2019

pedrobaeza commented Jul 18, 2019

hbrunn commented Jul 18, 2019

		@@ -1,4 +1,6 @@
		* Jay Vora <[email protected]>
		* Tecnativa <https://www.tecnativa.com>:

[12.0][MIG] web_widget_digitized_signature #1325

[12.0][MIG] web_widget_digitized_signature #1325

Conversation

max3903 commented Jul 5, 2019

pedrobaeza commented Jul 5, 2019

max3903 commented Jul 5, 2019

pedrobaeza commented Jul 5, 2019

pedrobaeza left a comment

Choose a reason for hiding this comment

pedrobaeza Jul 5, 2019

Choose a reason for hiding this comment

max3903 Jul 10, 2019

Choose a reason for hiding this comment

pedrobaeza Jul 10, 2019

Choose a reason for hiding this comment

pedrobaeza Jul 5, 2019

Choose a reason for hiding this comment

pedrobaeza Jul 5, 2019

Choose a reason for hiding this comment

pedrobaeza Jul 5, 2019

Choose a reason for hiding this comment

pedrobaeza Jul 5, 2019

Choose a reason for hiding this comment

max3903 commented Jul 10, 2019

pedrobaeza commented Jul 10, 2019

nikul-serpentcs commented Jul 13, 2019

max3903 commented Jul 16, 2019

pedrobaeza commented Jul 16, 2019

nikul-serpentcs left a comment

Choose a reason for hiding this comment

max3903 commented Jul 17, 2019

max3903 commented Jul 17, 2019

OCA-git-bot commented Jul 17, 2019

OCA-git-bot commented Jul 17, 2019

pedrobaeza commented Jul 17, 2019

rafaelbn commented Jul 18, 2019

max3903 commented Jul 18, 2019

rafaelbn commented Jul 18, 2019

pedrobaeza commented Jul 18, 2019

hbrunn commented Jul 18, 2019