Skip to content

[9.0] document_page_approval - #77

Merged
pedrobaeza merged 11 commits into
OCA:9.0from
naousse:9.0-document_page_approval
Jan 28, 2016
Merged

[9.0] document_page_approval#77
pedrobaeza merged 11 commits into
OCA:9.0from
naousse:9.0-document_page_approval

Conversation

@naousse

@naousse naousse commented Oct 28, 2015

Copy link
Copy Markdown
Contributor

No description provided.

@max3903

max3903 commented Oct 28, 2015

Copy link
Copy Markdown
Member

@naousse There are 2 modules in your pull request. Please split them in 2 branches and pull requests.

@max3903 max3903 added this to the 9.0 milestone Oct 28, 2015
Comment thread document_page_approval/README.rst Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/oder/other
add period

@max3903 max3903 mentioned this pull request Oct 30, 2015
9 tasks
@pedrobaeza pedrobaeza mentioned this pull request Oct 30, 2015
13 tasks
@max3903 max3903 changed the title document_page_approval migration from Odoo 8.0 to Odoo 9.0 [9.0] document_page_approval Oct 30, 2015
@max3903

max3903 commented Dec 8, 2015

Copy link
Copy Markdown
Member

@naousse
naousse force-pushed the 9.0-document_page_approval branch from 76cf59f to c518b37 Compare December 10, 2015 06:50
Comment thread document_page_approval/README.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naousse You have 2 extra '=' here.

@max3903

max3903 commented Dec 10, 2015

Copy link
Copy Markdown
Member

@naousse If you can write tests to remain over 80%, that would be enough for me to accept the module.

@naousse

naousse commented Dec 10, 2015

Copy link
Copy Markdown
Contributor Author

OK. I'm looking how to write some workflow test.
Please why does runbot failed

@max3903

max3903 commented Dec 10, 2015

Copy link
Copy Markdown
Member

@naousse It's not specific to your PR. It happened on all OCA ones during the last 24 hours. It seems to be a problem with the database. I will check it later today.

@max3903

max3903 commented Dec 11, 2015

Copy link
Copy Markdown
Member

👍

Comment thread document_page_approval/__init__.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use module docstrings

@max3903

max3903 commented Dec 23, 2015

Copy link
Copy Markdown
Member

@dreispt let's merge it ?

@max3903

max3903 commented Jan 13, 2016

Copy link
Copy Markdown
Member

@sysadminmatmoz, @gborelli can you review this PR please?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you decorate methods iterating on self for page in self with @api.multi?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the decorator @api.multi. But i don't know why it is necessary.

@max3903

max3903 commented Jan 13, 2016

Copy link
Copy Markdown
Member

Thanks @lmignon and @bealdav. I will work on that with @naousse.
Please RT the link to your PR you want me to review.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice as it, but in fact you can save bytes because in init you have no real code, just import as ascii. Then your file could only have 1 line.

@bealdav

bealdav commented Jan 13, 2016

Copy link
Copy Markdown
Member

@max3903 , I have to test to complet my review when runbot 'll have finished it build, but you can make a review here OCA/product-attribute#52, thanks
EDIT: runbot has failed

@lmignon

lmignon commented Jan 14, 2016

Copy link
Copy Markdown

@max3903 You already do so much for the community. My review is just a way to contribute to what the OCA give me for free.

@bealdav

bealdav commented Jan 14, 2016

Copy link
Copy Markdown
Member

@max3903 @lmignon, you're right so much work to animate community and publish info on https://odoo-community.org. Thanks to the whole community

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the code can be shortened

...
template = self.env.ref(''document_page_approval.email_template_new_draft_need_approval")
for page in self:
    if page.is_parent_approval_required:
        template.send_mail(page.id, force_send=True)
...

@lmignon

lmignon commented Jan 18, 2016

Copy link
Copy Markdown

@naousse can you chek in your code all the methods that iterate on self or expect that self is an active_record. In these cases you should add the appropriate decorator api.multi.

@lmignon

lmignon commented Jan 28, 2016

Copy link
Copy Markdown

👍 thank you @naousse

pedrobaeza added a commit that referenced this pull request Jan 28, 2016
@pedrobaeza
pedrobaeza merged commit 8c465bf into OCA:9.0 Jan 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants