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-20546: Multiple issues with creation of membership activities #10324

Merged
merged 6 commits into from
May 11, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented May 10, 2017

@jitendrapurohit jitendrapurohit changed the title CRM-20546: Multiple issues with creation of membership activities wip - CRM-20546: Multiple issues with creation of membership activities May 10, 2017
@jitendrapurohit jitendrapurohit changed the title wip - CRM-20546: Multiple issues with creation of membership activities CRM-20546: Multiple issues with creation of membership activities May 10, 2017
@petednz
Copy link

petednz commented May 10, 2017

@Stoob any chance you can also review since this issue has been a bit of pinball!

@eileenmcnaughton
Copy link
Contributor

@monishdeb aren't you working on this too?

@jitendrapurohit
Copy link
Contributor Author

@eileenmcnaughton No, actually the fix for CRM-20546 conflicted with already opened PR of Monish, so had a conversation with him yesterday and extended his PR #9468 to fix the extra comments mentioned by Pete #9468 (comment)

@joannechester
Copy link
Contributor

Tested and comments are at; https://issues.civicrm.org/jira/browse/CRM-20546

Copy link
Member

@monishdeb monishdeb left a comment

Choose a reason for hiding this comment

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

Thanks @jitendrapurohit for making additional fixes and extending desired unit tests. And thanks @joannechester for your review

@eileenmcnaughton
Copy link
Contributor

Merging - I agree with @monishdeb this seems to have been confirmed as an improvement by @joannechester and there is test coverage. I was going to say the code seems to be improved but we seem to have one long function extracted (good) but another long chunk of code added in another function. Perhaps that could have been extracted for readability. Anyway, I'm happy this has been reviewed & tested

@eileenmcnaughton eileenmcnaughton merged commit c77da34 into civicrm:master May 11, 2017
@jitendrapurohit jitendrapurohit deleted the CRM-20546 branch May 12, 2017 02:37
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.

6 participants