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-18081 Allow search of active relationships during a custom range of dates #10333

Merged
merged 3 commits into from
Jun 14, 2017

Conversation

francescbassas
Copy link
Contributor

@francescbassas francescbassas commented May 11, 2017

@jitendrapurohit
Copy link
Contributor

@francescbassas Any plan to write a unit test for the same ? You may find some related examples here - tests/phpunit/api//v3/RelationshipTest.php

@francescbassas
Copy link
Contributor Author

@jitendrapurohit sure, but I need some guidance. Which file should I to edit or create to write the unit test? Do you see the proposed changes well? I do not have much skill with English then would be nice if somebody reviewed the UI strings.

@jitendrapurohit
Copy link
Contributor

As the outcome shown on jira comment this PR works fine and gives the expected result. Maybe, we can optimize the calculation part to be in a single function for reports and searches.

For unit test, you can write it under tests//phpunit/CRM/Contact/BAO/QueryTest.php file. Normally we could create a formValue, project it in CRM_Contact_BAO_Query and assert the whereClause() formed. Also can ping on chat.civicrm.org for any doubts.

@francescbassas
Copy link
Contributor Author

Hi @jitendrapurohit, I optimized the calculation part to be in a a single function for reports and searchs and I renamed Active date to Active period. I tried to write the unit test, but I think it's too much for me.

@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented May 19, 2017

@francescbassas This is not working after the latest update. I even get relationship ending in 2015 when searching for relationship active in the year 2016. Also the qill message is missing. Did something went wrong in the optimization fix?

@francescbassas
Copy link
Contributor Author

@jitendrapurohit I got it! The issue appears renaming active_date field to active_period. Relative dates stop working. Renaming again active_period field to active_period_date fix the issue.

@jitendrapurohit
Copy link
Contributor

select2 widget broken on Advanced Search relationship tab after applying the updated patch. console is displayed as -

image

@francescbassas
Copy link
Contributor Author

francescbassas commented May 20, 2017

Yes, I miss the active_period to active_period_date rename on templates/CRM/Contact/Form/Search/Criteria/Relationship.tpl. I hope that now it works as expected. Thanks for your patience.

@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented May 20, 2017

message is displayed incorrectly in relationship reports if only from date is selected -

image

Also you can squash/fixup all your commits into single by following interactive mode. Basically what needs to be done is -

  1. In terminal -

     $ git rebase --interactive HEAD~23
    

    23 is no. of your total commits on this PR. Increment this number if you commit more. This will open an editor where all your last 23 commits are listed.

  2. Type fixup instead of pick written in front of each commit message(except the first one). Save it.

  3. Push forcefully -

     $ git push -f origin patch-10 <Enter> (patch-10 is your branch name)
    

@francescbassas
Copy link
Contributor Author

Done. I fixup all the commits.

The wrong message displayed in relationship reports if only from date is selected happens also with the other select 2 widgets dates 'Start Date' and 'End Date'. So I think it would be better to treat it as a separated bug.

}
elseif ($to) {
return CRM_Contact_BAO_Query::getRelationshipActivePeriodClauses(NULL, $to, FALSE);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the additional elseif()(line 821 - 826) here with && replaced by || in line 818 ? I think getRelationshipActivePeriodClauses() should take care of proper return clause as the check for $from & $to is done there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, see 02633eb

$dateValueHighFormated = date('Ymd', strtotime($dateValueHigh[2]));
$where[$grouping][] = self::getRelationshipActivePeriodClauses(NULL, $dateValueHighFormated, TRUE);
$this->_qill[$grouping][] = (ts('Relationship was active before')) . " " . CRM_Utils_Date::customFormat($dateValueHighFormated);
}
Copy link
Contributor

@jitendrapurohit jitendrapurohit May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here - Can we remove $where[$grouping][] statement from all the three condition and place it after if()... else if()... else if() (i.e after line 4228) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, see 02633eb

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works fine after the final changes.

@francescbassas
Copy link
Contributor Author

Thanks. I added some help to the advanced search form. Now are appearing failing checks but seems to be unrelated with my changes.

@francescbassas
Copy link
Contributor Author

jenkins, test this please

@francescbassas
Copy link
Contributor Author

@jitendrapurohit any plans to push it for 4.7.21?

@francescbassas
Copy link
Contributor Author

@colemanw or @eileenmcnaughton it is possible to push this for 4.7.21?

@colemanw colemanw merged commit 6afb57c into civicrm:master Jun 14, 2017
@colemanw
Copy link
Member

Code looks good and tested by Jitendra. Merging.

@francescbassas francescbassas deleted the patch-10 branch June 14, 2017 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants