[Trunk/7.0]Journals can not be deleted, even when they have no entries

The origin of these records seems to be in account/account_cash_statement.py around line 321:

class account_journal(osv.osv):
    _inherit = 'account.journal'

    def _default_cashbox_line_ids(self, cr, uid, context=None):
        # Return a list of coins in Euros.
        result = [
            dict(pieces=value) for value in [0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 20, 50, 100, 200, 500]
        ]
        return result

    _columns = {
        'cashbox_line_ids' : fields.one2many('account.journal.cashbox.line', 'journal_id', 'CashBox'),
    }

    _defaults = {
        'cashbox_line_ids' : _default_cashbox_line_ids,
    }

Worth noting that these values are wrong for many currencies. USD, for example, does not have $0.02, $0.20, or $200 pieces, and does have a $0.25 piece, but since I can't figure out what these records do, if anything, I can't say if this is actually a problem.

Hello kbh,

Found Invalid bug,

  * Create new journal and Save it.

  * During creation it also fill cash box, where 'currency denominations' are created for cash box.

  * You found this cash box list at ['Cash Registers' Tab], set checked cash control, you able to view filled cashbox.

  * Once you remove all related cash box entries, then after we easily able to unlink journal.

Right now I am invalidate this bug.

if I am wrong please correct me.

Thanks,
tta

Hello Tejas,

You might be getting wrong this issue.In this bug when we delete journal it shows integrity error which is not good.So we don't say that the journal can not be delete but it throws the integrity error instead of warning message.

We are not says that the journal not allow to delete but we wants a precious warning message inplace of integrity error. We have considered the integrity error as a bug in point of usability that's we was set this as a "Low" importance.

And yes, you are absolutely right the following step can delete the journal but it will shows on warning message.
 1.In journal Cash Registers tab >> checked cash control > you able to see related records.
 2.Remove all those records.
 3.Try to delete journal.

See I am giving a same example for you, please try to delete a account which is already reference with the invoice line then you will see warning message not the integrity error. So I am taking about this same way works for the journal.

I have attached the image which will clearly show this.

I think this reasons/description is quite enough for reopen this bug report, So I am going to reopen this again.

Thanks for the understanding!

Note: Please make sure before setting a bug as a invalid, If you are not damn sure then please put the comment and ask for the reply. Don't directly invalidate it.

Hello Khushboo,

Thanks for suggestion.

But as bug title and description says: "Journals can not be deleted, even when they have no entries ".
And what you suggesting now is different then the bug description.

 * In respect of contributor, please habituate to properly comment when you confirming the bug,
    so they may know what exactly is the problem.

Thanks for the understanding!

Hello Phil Frost,

Fixed :

 * It has been Fixed in https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-bug-1102612-tta

 * Revision ID: <email address hidden>

  It will be available soon in trunk.

Thanks,
Tejas

I see your change, and that error message is a little better. But, that's not what the bug is about. You are right, if I have a cash register assigned to the journal, I can go there and delete the cash control entries, and then I can delete the journal. But just changing the error message doesn't really address the issue, on several levels.

The error message in the proposed branch is: "You cannot remove an account that contains Cash Control items." What's a "cash control item?" I doubt I'd know, if I hadn't already figured it out by prodding around in the database. In fact, I'd find the old error message more useful, because at least it told me which table contained the thing that needed to be deleted. With the new error message, all I can do is wander around the interface looking for that "cash control" tab which is hidden three layers deep. A useful error message would tell me where to look.

But, what if there isn't a cash register associated with the journal? This is the case for the examples I gave, and is the case for most journals. Should I have to go create a cash register before I can delete a journal? Probably not.

The solution, I think, is to either not create cash control items for journals that are not associated with cash registers, or change this foreign key constraint to ON DELETE CASCADE. After all, what value is there in the cash control items if the journal does not exist? Or even better, implement both solutions.

I'd also say this approach of trying to cover every individual case of a user action violating with a special error message is a bad idea. See this earlier bug I reported:

https://bugs.launchpad.net/openobject-addons/+bug/1096439

The issue is that there are probably *thousands* of places where a user could delete or modify something and violate a database constraint. If you try to write code to handle each one individually, you will miss some for sure. Worse, some of them will be wrong, as in that bug.

And, the code that has now been put everywhere will be more complicated and hard to read, and it's just doing the same thing the database should be doing anyway. Besides being pointless, it's inefficient. With the proposed change, there must be an additional database query, and the data must go through all the python code and the ORM. Why do all this work, when the database will check it anyway? PostgreSQL, written in C, without the network overhead, without all the ORM logic, will be at least 100x faster.

A better approach would be to attempt the operation, and let the database raise an error if there's a problem, then handle *that* error. Or better, inform the database on ways to handle the problem itself, like in this case, making the constraint ON DELETE CASCADE. That will be much cleaner and faster.

The ondelete=cascade will do the trick. I commented on the merge proposal and a new one will soon arrive.

Thanks for your contribution Phil

fix released in addons v7, on revision 8710 revision-id: <email address hidden>

Thanks for the contribution!
Quentin