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

Fixes issue with duplicate is_billing on inline address forms. #22850

Merged
merged 3 commits into from
Apr 7, 2022

Conversation

VangelisP
Copy link
Contributor

Overview

In the case that someone has 2 addresses registered, they can flag both of them as billing location via the inline address editing checkbox which I believe is wrong. This behaviour does not replicate when you edit the contact.
I am assuming that this is not an intended behaviour. Source Issue: https://lab.civicrm.org/dev/core/-/issues/3080

Before

No validation takes place when someone checks the "billing location" via the inline address editing which means that a contact can have multiple billing addresses.

After

There should be a validation while saving the inline address form to check if the checkbox is already being used in another address ID and if yes, it should not store the checkbox (or unset it).

@civibot
Copy link

civibot bot commented Feb 28, 2022

(Standard links)

@civibot civibot bot added the master label Feb 28, 2022
@VangelisP
Copy link
Contributor Author

VangelisP commented Feb 28, 2022

In regards to the failed tests, I can't seem to find any relevance with my code or why my code is causing these errors. If I'm not mistaken, they refer to something else.

If someone can shed some light, I would highly appreciate it !

@demeritcowboy
Copy link
Contributor

At line 452 you are creating a brand new address record - was that intended to be something else?

@VangelisP
Copy link
Contributor Author

At line 452 you are creating a brand new address record - was that intended to be something else?

Ohhh ... 🤦‍♂️ I definitely forgot to remove those 4 lines of code from the function that I repurposed!

Sharp eyes @demeritcowboy , I'll issue a fix right now.

@VangelisP
Copy link
Contributor Author

Squashed commits

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • I only ran via the UI. So I'm only putting merge-ready because someone might have some unusual api usage this affects.
      • If you fill out a contribution page, it will overwrite the existing billing address for an existing contact, but this happens before the patch too and is probably already a known thing and maybe even people want that(?). In any case it's no change in how that works.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • It seems a bit complex but I know it's copy/paste from handlePrimary().
    • [r-maint] ?
      • A test would be a nice-to-have. That might also help confirm api usage.
    • [r-test] PASS

@demeritcowboy demeritcowboy added merge ready PR will be merged after a few days if there are no objections needs-test labels Mar 8, 2022
@jaapjansma
Copy link
Contributor

@eileenmcnaughton can you merge this?

@eileenmcnaughton
Copy link
Contributor

@jaapjansma I did see your last comment but this to my mind absolutely requires a unit test & I can't personally merge it without one. I did merge another one today that had been marked as not requiring a test even though I felt uncomfortable - but this is deep in the BAO layer and we already have tests around the matching is_primary functionality.

I did mentally add it to my 'long-list' of things to do last time as I understand requests to merge things that I can't merge without a test as a request that I write the unit test.

@jaapjansma
Copy link
Contributor

@VangelisP are you able to provide a unit test? As per @eileenmcnaughton and @demeritcowboy comment?

@VangelisP
Copy link
Contributor Author

While I haven't written a unit test before, I totally understand you and I am willing to give it a try, heading for a similar test as with is_primary on the AddressTest.php , am I right?

@eileenmcnaughton
Copy link
Contributor

@VangelisP yep!

@VangelisP
Copy link
Contributor Author

Although I am not quite sure why this failed nearly instantly, this is my take on the PHPUnit test with is_billing. Let me know if that works for you.

I tried without the patch and it fails (obviously) and with the patch, it succeeds.

What fails? Assigned into a contact 2 addresses with is_billing set to 1, then tried to detect if is_billing is detected only once (which is correct).

@demeritcowboy
Copy link
Contributor

Thanks for the test. There's fails on all the PRs right now since something is wrong with the server.

@eileenmcnaughton eileenmcnaughton added merge on pass and removed merge ready PR will be merged after a few days if there are no objections needs-test labels Apr 6, 2022
@demeritcowboy
Copy link
Contributor

jenkins retest this please

@colemanw colemanw merged commit 495e2f9 into civicrm:master Apr 7, 2022
@colemanw
Copy link
Member

colemanw commented Apr 7, 2022

Thanks for the fix @VangelisP

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.

5 participants