[16.0][IMP]subscription_oca: Various improvements (ready for review) #1098
Conversation
|
concern #1057 |
flotho
force-pushed
the
16-imp-subscription_oca
branch
from
aa35e16
to
2cef796
Compare
|
Hi @tarteo @rousseldenis here is my proposal to supersedes #1058 |
|
ping @JulienMartinez @gaelTorrecillas @GuiPrime , ready for review please |
flotho
force-pushed
the
16-imp-subscription_oca
branch
2 times, most recently
from
153adad
to
e18a916
Compare
flotho
changed the title
[IMP] subscription_oca: Various improvements oca_subscription: Open subscription button in invoice view [IMP]subscription_oca: syntax as requested [IMP]subscription_oca: non used field on stage [IMP]subscription_oca: track amount changes [REF]subscription_oca: more consistent stage and status management [IMP]subscription_oca: add views * Invoices menu * Filter entries in search views [IMP]subscription_oca: Contributors [IMP]subscription_oca: ACL issue on contact [IMP]subscription_oca: use code as invoice reference [IMP]subscription_oca: unit tests [LINT]subscription_oca [LINT]subscription_oca [LINT]subscription_oca [FIX]subscription_oca: authoring [IMP] subscription_oca: Various improvements
flotho
force-pushed
the
16-imp-subscription_oca
branch
from
e18a916
to
63ab1cf
Compare
| @@ -464,7 +501,7 @@ def create(self, values): | |||
| values["date_start"] = values["recurring_next_date"] | |||
| values["stage_id"] = ( | |||
| self.env["sale.subscription.stage"] | |||
| .search([("type", "=", "pre")], order="sequence desc", limit=1) | |||
| .search([("type", "=", "draft")], order="sequence desc", limit=1) | |||
There was a problem hiding this comment.
I think this is not longer needed with the addition of the _get_default_stage_id method
There was a problem hiding this comment.
And i also think that the first conditional of the create function could be removed because it is unreachable code
| and subscription.sale_subscription_line_ids | ||
| ): | ||
| try: | ||
| subscription.generate_invoice() | ||
| except Exception: | ||
| logger.exception("Error on subscription invoice generate") | ||
| if not subscription.recurring_rule_boundary: | ||
| if subscription.date == today: | ||
| if subscription.date <= today: | ||
| subscription.action_close_subscription() |
There was a problem hiding this comment.
I think this should be replaced with subscription.close_subscription() and a default reason, isn't it?
The subscription.action_close_subscription() only returns a wizard and it causes no effect
| domain="[('in_progress', '=', True)]" | ||
| /> | ||
| <separator /> | ||
| <!-- TODO: to_renew seems useless in this module --> |
There was a problem hiding this comment.
I algo think to_renew is useless currenly. But i also think adding the field to the form view, or deleting it from the models and the search is better than adding a TODO
| /> | ||
| <separator /> | ||
| <filter | ||
| string="Draft / Waiting" |
There was a problem hiding this comment.
This filter also shows the closed subscriptions.
Maybe we should add the related stage_type field to the subscription and create better filters?
|
on holidays, will deal with this later ;-) |
Enjoy! 🏖 |
|
@flotho Can you also add a sequence field to sale.subscription.line? |
There was a problem hiding this comment.
I don't care if yours or mine gets merged, so I approve it. I preferably see this and the contract modules refactored mentioned here: #1108 but that will be a substantial amount of work...
If you can just fix the issue @Tisho99 mentioned, this can be merged. Frankly, any merged pull request is a big win for this module
supersedes #1058