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-21126: fix default selection of member_is_primary input #10923

Merged
merged 2 commits into from
Aug 31, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Aug 30, 2017

Overview

default selection of member_is_primary input is not done on passing param through URL.

Before

Passing owner in the url does not enable any option in member_is_primary radio input.

Eg. http://dmaster.demo.civicrm.org/civicrm/member/search?reset=1&force=1&status=1,2,3&type=12&owner=1 screen would not select Primary Member? input in the membership search form. This would lead to incorrect contacts in export and various task.

image

After

Primary Member? is correctly selected.


@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit this seems OK & I guess @lcdservices probably has a vested interest here. But, I think perhaps we should cast $owner to 1 or 0 ? e.g

$this->_defaults['member_is_primary'] = ( $owner ? 1 : 0);

@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Aug 31, 2017

The above would set Primary Member? to No even if owner param is not present in the url.

Eg. http://dmaster.demo.civicrm.org/civicrm/member/search?reset=1&force=1 would still check the No option.

Shouldn't we clear this field if we don't have any valid value (0 or 1) for owner? Hence the is_null() check.

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit yep the isNull check is good - but if not null then it is binary - ie. 0 = 0, 1=1 , 2=1, twentyfivefrogs=1

so I am just suggesting making that more expicit

@jitendrapurohit
Copy link
Contributor Author

Have done so now 👍

@eileenmcnaughton
Copy link
Contributor

Seems fine to me now. Let's merge on pass

@monishdeb monishdeb merged commit 111accc into civicrm:master Aug 31, 2017
@jitendrapurohit jitendrapurohit deleted the CRM-21126 branch September 6, 2017 03:06
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.

4 participants