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

CRM-13725 check custom fields to test for duplicate relationship. #6515

Conversation

Projects
None yet
6 participants
@johanv
Copy link
Contributor

commented Aug 16, 2015

johanv added some commits Aug 16, 2015

CRM-13725 - Duplicate relationship should consider custom fields, uni…
…t test.

----------------------------------------
* CRM-13725: Make check for duplicate relationship aware of custom fields
  https://issues.civicrm.org/jira/browse/CRM-13725
CRM-13725 - Check all custom fields to prevent dupe relationships.
----------------------------------------
* CRM-13725: Make check for duplicate relationship aware of custom fields
  https://issues.civicrm.org/jira/browse/CRM-13725

@kurund kurund added the 4.7 label Aug 16, 2015

@colemanw

This comment has been minimized.

Copy link
Member

commented Aug 16, 2015

@johanv looks like there were some test failures generated from that new checkDuplicateCustomFields function.

CRM-13725 - don't look in custom values if there aren't any.
----------------------------------------
* CRM-13725: Make check for duplicate relationship aware of custom fields
  https://issues.civicrm.org/jira/browse/CRM-13725
@johanv

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

I think I fixed the problem with the unit tests. But while I was doing this, I realized that there might be a problem if you add a relationship without custom values, and a similar relationship exists with custom values.

@colemanw

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

Could you add a unit test for that scenario?

@kurund

This comment has been minimized.

Copy link
Member

commented Sep 4, 2015

@johanv any progress on unit tests

CRM-13725 - Extra (failing) unit tests.
If you have an existing relationship and a new relationship,
and the new relationship has the same standard fields than the existing one,
but only one of the two relationships has custom fields, CiviCRM
will not accept the new relationship, because it will consider the
new one as a duplicate.

Two unit tests, they will obviously fail.

----------------------------------------
* CRM-13725: Make check for duplicate relationship aware of custom fields
  https://issues.civicrm.org/jira/browse/CRM-13725
@johanv

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2015

I just added two tests. One for an existing relationship with custom fields, and a new one without custom fields. And one for an existing relationship without custom fields, and a new one with custom fields.

They both fail. I guess the function checkDuplicateCustomFields (part of the PR) has to be fixed, but it is not always clear what the behaviour should be. Suppose that the existing relationship has no custom fields, and the new one has, but with all custom values null or empty. I guess this should be considered as a duplicate?

@colemanw

This comment has been minimized.

Copy link
Member

commented Sep 5, 2015

I think you will always be comparing relationships that are of the same
type, therefore they will always have the same custom fields defined
(granted they might not have ever been entered so the sql row for them
might not exist, which is slightly different than the row existing but
with null or empty values). But I think you are correct - for purposes
of deduping, NULL == EMPTY == MISSING.

On 09/05/2015 11:39 AM, Johan Vervloet wrote:

I just added two tests. One for an existing relationship with custom
fields, and a new one without custom fields. And one for an existing
relationship without custom fields, and a new one with custom fields.

They both fail. I guess the function checkDuplicateCustomFields (part
of the PR) has to be fixed, but it is not always clear what the
behaviour should be. Suppose that the existing relationship has no
custom fields, and the new one has, but with all custom values null or
empty. I guess this should be considered as a duplicate?


Reply to this email directly or view it on GitHub
#6515 (comment).

johanv added some commits Sep 6, 2015

CRM-13725 This should cover the case of missing custom fields.
----------------------------------------
* CRM-13725: Make check for duplicate relationship aware of custom fields
  https://issues.civicrm.org/jira/browse/CRM-13725
CRM-13725 - check whether $params['custom'] exists before using it.
----------------------------------------
* CRM-13725: Make check for duplicate relationship aware of custom fields
  https://issues.civicrm.org/jira/browse/CRM-13725
@johanv

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2015

Still two errors. But I don't really understand what's going wrong here.

@totten

This comment has been minimized.

Copy link
Member

commented Sep 7, 2015

So I haven't fully traced, but maybe this can point in the right direction:

  • The error output includes two traces. This is a bit confusing.
  • The first trace+error_message seems to be the outermost API call -- MembershipType.getcount.
  • Which makes a nested API call to MembershipType.get and then returns/outputs any resulting errors.
  • The second trace+error_message seems to be from MembershipType.get (with the extra parameter is_count). The error message there is Invalid argument supplied for foreach().
  • Guess: MembershipType.get then calls _civicrm_api3_get_using_utils_sql, where there's a conditional which checks for is_count and then runs a foreach loop. Maybe there's something wrong in this area?
@johanv

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2015

I had similar failures with an unrelated PR: #6747. Maybe the failures are caused by something else?

@davecivicrm

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2015

@colemanw pretty sure @johanv is correct and test failures are unrelated since they occur on all recent PRs on master. However, not merging myself since I'm not sure if the prior threads are resolved.

@davecivicrm

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2015

Jenkins test this please.

@colemanw

This comment has been minimized.

Copy link
Member

commented Nov 6, 2015

jenkins, retest this please

colemanw added a commit that referenced this pull request Nov 7, 2015

Merge pull request #6515 from johanv/CRM-13725-duplicate_realtionship…
…s_check_custom_fields

CRM-13725 check custom fields to test for duplicate relationship.

@colemanw colemanw merged commit c219b4c into civicrm:master Nov 7, 2015

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.