#886 company_save updates wrong row in company

1.3
closed-fixed
nobody
None
1
2013-09-12
2013-08-15
John Locke
No

09:03 <Ramjett> good morning
09:10 <Ramjett> I think I have a couple of pretty major bugs, or something weird, in 1.3.33 . I'm hoping it really isn't corrupting data that I can't fix
09:13 <Ramjett> In my AR --> Customers --> Reports --> Search ... My list is showing duplicates and missing AR customers. The duplicates are the missing AR customers IDs
10:34 <freelock_> Ramjett: there aren't any schema changes in stock lsmb within 1.3.x, you can try going back to 1.3.32 and re-run setup.pl
10:34 <freelock_> then see if your data is intact
10:34 <freelock_> I've tracked down a few regressions this way, using git bisect to find the commit that introduces a problem
10:34 <Ramjett> I'm looking in the database and there are dups
10:35 <Ramjett> like it changed
10:35 <freelock_> in eca?
10:35 <Ramjett> legal_name in company
10:36 <freelock_> checking my database...
10:37 <Ramjett> I could be from text edits.
10:37 <Ramjett> in fact I'm pretty sure
10:37 <Ramjett> but not sure how to fix
10:37 <freelock_> so what screen are you making these edits?
10:38 <freelock_> I'm not seeing duplicates in mine, but the last one I added was a month ago
10:38 <Ramjett> I'm getting steps
10:42 <Ramjett> AR -- > Customers --> Reports --> Search --> bring up list -- choose name of company -- click on company -- edit name -- save
10:45 <freelock_> huh, that's definitely a bug...
10:46 <freelock_> I'm not seeing any duplications in entity or entity_credit_account
10:47 <Ramjett> I have not looked at entity .. looking
10:49 <freelock_> yeah, it looks like it's only adding rows to company
10:49 <freelock_> they both still have the same entity_id
10:49 <freelock_> and entity_credit_account is related to entity, not to company
10:49 <Ramjett> I do not see dup there
10:49 <freelock_> I just deleted the dupe in company, and not seeing any side effects
10:50 <Ramjett> that sounds scary :-)
11:03 <freelock_> well, as I understand the entity system, it's entity and entity_credit_account that are critical and widely used -- mostly eca.
11:03 <Ramjett> I'm trying to see what it over wrote .. my company id do not have dup id and they are in order .. just the legal_name has dup
11:05 <freelock_> most of what you see on invoices are the eca.pay_to_name
11:06 <freelock_> I also find that in most cases, there are foreign keys in use, at least on all the new tables
11:06 <freelock_> if there's data in other tables relying on a row you're deleting, postgres won't let you delete it without CASCADE
11:08 <Ramjett> I just do not want to let everyone use when it might be putting in things wrong
11:12 <freelock_> sure... do you know if this is a regression? I'm not in the habit of editing on the Company tab, I only really edit the Accounts
11:15 <Ramjett> regression ?
11:17 <freelock_> something that used to work but is now broken
11:20 <freelock_> Ok, I definitely see the problem, and my guess is, this is not a regression, but something that always behaved this way (since moving to this screen)
11:21 <freelock_> the form posts the entity_id as "id"
11:21 <Ramjett> not sure, I have only been using ledgesmb for a few months. I was using sql-ledger
11:21 <freelock_> the stored function company_save taked id as its first argument
11:21 <freelock_> in my case, entity_id is 786, company.id is 450
11:22 <freelock_> company_save updates company row with the wrong id, and fails, so it inserts a new row
11:22 <Ramjett> I see in company my entity_id id are not correct
11:22 <Ramjett> in the ones that I am told that were edited
11:23 <freelock_> youch, that means there was probably a collision
11:24 <freelock_> what I think I would do is patch sql/modules/Company.sql, line 681
11:25 <freelock_> change: WHERE id = t_company_id;
11:25 <freelock_> to: WHERE entity_id = t_entity_id;
11:26 <freelock_> then go to setup.pl and reload the scripts
11:26 <freelock_> and then you'll have some data cleanup to do...
11:26 <freelock_> actually, this might clean up your data:
11:27 <Ramjett> I'll try it on a copy in a little bit
11:27 <Ramjett> I'm still trying to wrap my head around what is happening
11:28 <Ramjett> So you think it is only corrupted the company table ?
11:31 <freelock_> update company set legal_name = ( select name from entity where company.entity_id = entity.id)
11:32 <freelock_> that should get the names and entity ids straightened up
11:32 <freelock_> sic data and tax ids are likely totally messed up, if you use those columns
11:32 <Ramjett> I use tax id
11:33 <freelock_> what's happening is the company_save is updating the company record using the wrong id
11:33 <freelock_> it's using the entity_id as the company.id
11:33 <freelock_> so if those don't match up, it's updating the wrong record
11:34 <freelock_> if there is no matching company.id, it inserts a new row, and thus you get duplicates
11:35 <Ramjett> well it does not look like it inserted any rows, just updated with wrong id
11:35 <Ramjett> but kept the old name so .. that is the dup
11:36 <freelock_> ah.
11:36 <freelock_> company_save does update legal_name, sic_code, and tax_id, so any that got updated incorrectly, clobbered those previous values.
11:36 <freelock_> I'm not seeing tax_id or sic_code elsewhere, so I'm guessing that data is lost
11:37 <freelock_> (unless you can restore from an earlier backup)

Discussion

  • John Locke
    John Locke
    2013-08-15

    Markdown hid the authors of each line from IRC -- conversation between Ramjett and Freelock

     
  • John Locke
    John Locke
    2013-08-15

    Committed fix for 1.3.x in #5943. Not tested, or ported to trunk.

     
  • John Locke
    John Locke
    2013-08-15

    Note that this code could probably use broader cleanup.

    In my review of what was happening, I found the company form had a hidden "id" field that was set to the entity_id value, NOT the company.id value. This was posted to the controller, which happily set the id to the entity_id for the company_save function, which then proceeded to update the wrong row.

    My fix is to entirely ignore company.id, and use entity_id instead, which seems to be far more reliable.

    Is there a reason for this table to exist, or should it get merged into entity? (suggestion for 1.5?)

     
  • John Locke
    John Locke
    2013-08-15

    Just checked trunk, and it looks like Chris applied the exact same fix in #5761 back in May.

     
  • Chris Travers
    Chris Travers
    2013-09-12

    • status: open --> closed-fixed
     
  • Chris Travers
    Chris Travers
    2013-09-12

    Closing as it has been fixed in 1.3 and trunk. A note on code cleanup: this was some of the least clean code we introduced in 1.3 and it was significantly cleaned up in trunk.

     
  • Chris Travers
    Chris Travers
    2013-09-12

    (I expect the code will be even further cleaned up in 1.5 but need to think about how, since there is a lot of irreducible complexity here)