Code review comment for lp:~pedro.baeza/account-financial-tools/7.0-account_chart_update-enhanced

Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Yannick,

Thanks for taking the time to review such a huge change that I have made on the module. I know that there's still a lot to do to comply with all the conventions, but this is a beginning motivated by some requirements not met in it.

I have fixed things you have point about missing contexts.

About tax_template to tax_templ it was to favour PEP8 80 cols restriction, because with that long names was very difficult to fit lines. I think it's still enough readable and I win 3 chars less.

I removed required=True from fill to avoid a strange situation that happens when removing a chart template (a strange user case, but I faced it in one case): if you put this field as required, and you run the wizard, a row is created on the temporary table form the transient model. This table is not clean up until next vacuum. Meanwhile, if you try to remove the chart template that has been selected on the wizard, due to ondelete="cascade" attribute, chart_template_id is emptied on temporary table, but required constraint throws an error, avoiding you to remove this chart template. That's why I have moved required condition to view.

Regards.

« Back to merge proposal