dev/core/issues/842, Fixed code to update website rather delete

[pradpnayak]

Overview

https://lab.civicrm.org/dev/core/issues/842

Before

Website gets deleted when updated

After

Website is updated

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

@pradpnayak are you able to add a unit test to this and do you have any idea of when it broke?

[pradpnayak]

@seamuslee001 this was reported in https://civicrm.stackexchange.com/questions/29114/inline-editing-of-website-field-deletes-it which can be replicated on >= 5.10

Not sure but it may be related to these commits #13169 and #13170

[eileenmcnaughton]

@pradpnayak this has been a really fragile bit of functionality :-( Would be v grateful if you can add a test so we can hopefully break the cycle

[eileenmcnaughton]

@elisseck are you able to review?

[pradpnayak]

Hi @eileenmcnaughton

This issue is more of UI so do you want me to add unit test for UI submit or just for the CRM_Core_BAO_Website::process() function?

Thanks
Pradeep

[eileenmcnaughton]

@pradpnayak I had assumed the latter - does that work out or is it kinda pointless?

[elisseck]

@eileenmcnaughton Yes i'll be happy to review this since my recent PRs touched this functionality (although i'm not 100% sure how they would have caused this :) ). Should be able to get to it tonight.

[elisseck]

Okay I tested this on a buildkit build of current 5.13.alpha1 and i'm seeing some really strange behavior that I don't understand. The behavior described in the issue is not consistent at all for me. Here is a little screencast you'll see a failure when I add the 4th website but you'll see a bunch of successes as well. This is mostly FYI for writing this test I think it's going to be confusing:

However weird this is... checking out the PR does seem to resolve this issue after editing websites for a few minutes I saw no more issues... so my review is as follows:

• General standards
  • (r-explain) PASS
  • (r-user) PASS
  • (r-doc) PASS
  • (r-run) PASS
• Developer standards
  • (r-tech) PASS
  • (r-code) PASS
  • (r-maint) PASS
  • (r-test) Undecided: It would be great to see the test as discussed above although I can't say much there because i'm definitely guilty of not writing tests for my previous PRs (sorry!!).

[eileenmcnaughton]

@elisseck caught out huh :-) But thanks for doing the review

[mieg]

This fixes the issue for me, on 5.11.0 / Drupal 7.65. Thanks @pradpnayak!

[colemanw]

I think this needs to be merged into 5.12.

[eileenmcnaughton]

I've put this against 5.12 #13939

(perhaps we can still get that test against master :-)

[eileenmcnaughton]

Closing as merged through via 5.12 - would be happy to see that test though :-)