Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v9 Migration of sale_cancel_reason #261

Merged
merged 7 commits into from Nov 22, 2017
Merged

Conversation

JayVora-SerpentCS
Copy link
Sponsor Contributor

No description provided.

@pedrobaeza pedrobaeza mentioned this pull request Jan 29, 2016
50 tasks
@pedrobaeza
Copy link
Member

Can you please squash the commits a bit?

'complexity': 'normal',
'images': [],
'website': "http://www.camptocamp.com",
'description': """
Sale Cancel Reason
Copy link
Member

Choose a reason for hiding this comment

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

@tafaRU
Copy link
Member

tafaRU commented Jan 29, 2016

Please also have a look at https://travis-ci.org/OCA/sale-workflow/builds/105690296 errors.
By the way, thank you for your contrib!

@oca-clabot
Copy link

Hey @JayVora-SerpentCS, 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:

  • @akashpatel3193 (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

Please check.

@pedrobaeza
Copy link
Member

You must import the commit history from the migration moment following this guide: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-9.0 and squash commits to only relevant ones.

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

Sure, will do! Sounds pretty good set of notes (y)


* To configure this module, you need to:

* You can cofigure cancel reason from Sale -> Configuration -> Sale Order Cancel Reason.
Copy link
Member

Choose a reason for hiding this comment

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

n/cofigure/configure

@atchuthan
Copy link
Member

code reviewed 👍
few minor changes and rebase commits from 8.0 as per @pedrobaeza request

</form>
</field>
</record>
<data noupdate="0">
Copy link
Member

Choose a reason for hiding this comment

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

<odoo>, not <data>

Copy link
Member

Choose a reason for hiding this comment

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

@JayVora-SerpentCS would you be able to fix this misunderstanding? Like @pedrobaeza says, the data tag is redundant in 9.0, so please replace with the odoo tag.

@@ -28,8 +29,7 @@ class SaleOrder(models.Model):
cancel_reason_id = fields.Many2one(
'sale.order.cancel.reason',
string="Reason for cancellation",
readonly=True,
ondelete="restrict")
Copy link
Member

Choose a reason for hiding this comment

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

By removing the ondelete behaviour the default 'set null' will apply. Reasons that were applied to sale orders can be removed, leaving the value empty. Is that what you want?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

surely

Copy link
Member

Choose a reason for hiding this comment

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

More common is not to allow this, but to allow reasons to be set to inactive so that they cannot be assigned anymore.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Added an active field to manage this.
Thanks.

@StefanRijnhart
Copy link
Member

@JayVora-SerpentCS should be very easy to have this in a mergeable state, if you have a couple of minute. Nice description page!

@pedrobaeza
Copy link
Member

Any update of this?

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

JayVora-SerpentCS commented Nov 11, 2016

Will find some tome and push

@pedrobaeza
Copy link
Member

Are you going to finally work on this?

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

Sorry, lost it. doing in this week for sure!

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

You may proceed please.

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

@pedrobaeza I am unable to see what fails on runbot. Can you help please?

@StefanRijnhart
Copy link
Member

@JayVora-SerpentCS I think it's the warnings raised by sale_rental. Don't worry about it.

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

Kindly check.

@pedrobaeza
Copy link
Member

Runbots errors are in fact warnings from other module (sale_rental):

2017-01-11 13:49:54,128 216 WARNING openerp_test openerp.models: method product.product._check_rental: @constrains parameter 'must_have_dates' is not writeable
2017-01-11 13:49:54,128 216 WARNING openerp_test openerp.models: method product.product._check_rental: @constrains parameter 'type' is not writeable
2017-01-11 13:49:54,128 216 WARNING openerp_test openerp.models: method product.product._check_rental: @constrains parameter 'uom_id' is not writeable

:target: https://www.gnu.org/licenses/agpl.html
:alt: License: AGPL-3

Sale Cancel Reason
Copy link
Member

Choose a reason for hiding this comment

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

Put the same lines above

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Some tests would be welcome.

This module introduce the following features:

* This module allows cancel sale order and reason must be given.

Copy link
Member

Choose a reason for hiding this comment

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

Don't put this extra line

=============

* To configure this module, you need to:

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line and replace * by #. for numbered list.


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/167/8.0
Copy link
Member

Choose a reason for hiding this comment

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

s/8.0/9.0

:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/167/8.0

For further information, please visit:
Copy link
Member

Choose a reason for hiding this comment

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

Remove this paragraph

Maintainer
----------

.. image:: http://odoo-community.org/logo.png
Copy link
Member

Choose a reason for hiding this comment

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

s/http/https in both places

'name': 'Sale Cancel Reason',
'version': '9.0.1.0.0',
'author': 'Camptocamp, Odoo Community Association (OCA), '
'Serpent Consulting Services Pvt. Ltd.',
Copy link
Member

Choose a reason for hiding this comment

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

Fix indentation

<record id="cancel_reason_other_provider" model="sale.order.cancel.reason">
<field name="name">Other Service Provider selected</field>
</record>
</odoo>
Copy link
Member

Choose a reason for hiding this comment

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

Line at the end

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

This is done, I wonder why the diff shows it open.

@@ -29,11 +30,15 @@ class SaleOrder(models.Model):
'sale.order.cancel.reason',
string="Reason for cancellation",
readonly=True,
ondelete="restrict")
ondelete='restrict')
Copy link
Member

Choose a reason for hiding this comment

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

Next time, don't make these stylistic changes that increases diffs.

@@ -3,6 +3,7 @@
#
# Author: Guewen Baconnier
# Copyright 2013 Camptocamp SA
# Copyright 2016 Serpent Consulting Services Pvt. Ltd.
Copy link
Member

Choose a reason for hiding this comment

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

Don't add your copyright on non touched files (and more if they are __init__.py files.

@JayVora-SerpentCS
Copy link
Sponsor Contributor Author

Will do the changes asap.

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Thanks for the work! Some comments in line.

Installation
============

No external library is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove unneeded sections. Also this is displayed as a quotation due to the initial space in the line.

=============

* To configure this module, you need to:
*#. You can configure cancel reason from Sale -> Configuration -> Sale Order Cancel Reason.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove initial asterisks, this is not going to be displayed well.

'Serpent Consulting Services Pvt. Ltd.',
'category': 'Sale',
'license': 'AGPL-3',
'complexity': 'normal',
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed I think

],
'installable': False,
}
'test': ['test/sale_order_cancel.yml'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the tests to python?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Sure

@@ -1,14 +1,13 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
<data noupdate="1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the original noupdate="1"?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Accidental due to removal of , its back now.

</div>
</div>
</div>
</section>
Copy link
Contributor

Choose a reason for hiding this comment

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

Final extra line

<h3 class="oe_slogan">It is chosen from a configured list</h3>
<div class="oe_span6">
<p class="oe_mt32">
1) Sale Order Configuretion.
Copy link
Contributor

Choose a reason for hiding this comment

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

Configuretion - > Configuration

}
'installable': True,
'application': True,
'auto_install': False,
Copy link
Member

Choose a reason for hiding this comment

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

@JayVora-SerpentCS not necessary, and can you also please add OCA icon in static/description

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@sudhir-serpentcs
Copy link

@StefanRijnhart @lreficent @pedrobaeza Please review and provide feedback if any.

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Tested on runbot 👍

Copy link
Member

@serpentcs-dev1 serpentcs-dev1 left a comment

Choose a reason for hiding this comment

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

Code review LGTM 👍

@pedrobaeza pedrobaeza merged commit 8b5c853 into OCA:9.0 Nov 22, 2017
norlinhenrik pushed a commit to loymcom/sale-workflow that referenced this pull request Sep 20, 2023
* [10.0] re-introduce state field on product

* [ADD] add setup for product state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants