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

Removed hardcoded activity status and used Activity create #14720

Merged
merged 1 commit into from
Jul 7, 2019

Conversation

pradpnayak
Copy link
Contributor

Overview

Removed hardcoded activity status and used Activity create
Before

Activity status was hardcoded to 1

After

Used activity status name and api to create activity

@civibot
Copy link

civibot bot commented Jul 3, 2019

(Standard links)

@civibot civibot bot added the master label Jul 3, 2019
@pradpnayak
Copy link
Contributor Author

jenkins retest this please

@colemanw
Copy link
Member

colemanw commented Jul 3, 2019

This looks good to me, but as far as I can tell this function is not covered by unit tests. Can someone test it manually?

@pradpnayak
Copy link
Contributor Author

@colemanw have pushed unit test for this.

@pradpnayak
Copy link
Contributor Author

jenkins retest this please

@colemanw
Copy link
Member

colemanw commented Jul 4, 2019

@civicrm-builder retest this please

'activity_type_id' => 'Bulk Email',
'status_id' => 'Completed',
'subject' => 'Example Subject',
], 1);
Copy link
Member

Choose a reason for hiding this comment

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

I've seen tests fail when run multiple times in situations like this because after the 1st run there would be more than one activity named "Example Subject".
I like to get around this by doing something like $subject = uniqid('actCreateTest');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @colemanw
Just pushed the change

@colemanw
Copy link
Member

colemanw commented Jul 5, 2019

This looks good @pradpnayak.
Could you please squash the 3 commits in this PR?

Added unit test

Use unique subject for activity in test
@pradpnayak
Copy link
Contributor Author

@colemanw have squashed the commits.

@eileenmcnaughton eileenmcnaughton merged commit d66bdc3 into civicrm:master Jul 7, 2019
@pradpnayak pradpnayak deleted the REF-1 branch July 7, 2019 18:34
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.

3 participants