-
-
Notifications
You must be signed in to change notification settings - Fork 824
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-20425 - Configure inbound email activity status #10159
Conversation
colemanw
commented
Apr 13, 2017
•
edited by civicrm-builder
Loading
edited by civicrm-builder
- CRM-20425: Activity status per mail account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left a few comments, mostly questions or minor suggestions about line length.
CRM/Admin/Form/MailSettings.php
Outdated
@@ -85,6 +85,7 @@ public function buildQuickForm() { | |||
0 => ts('Email-to-Activity Processing'), | |||
); | |||
$this->add('select', 'is_default', ts('Used For?'), $usedfor); | |||
$this->add('select', 'activity_status_id', ts('Activity Status'), CRM_Activity_BAO_Activity::buildOptions('status_id')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, but line could be shortened by splitting buildOptions result into a separate variable.
CRM/Admin/Form/MailSettings.php
Outdated
public function setDefaultValues() { | ||
$defaults = parent::setDefaultValues(); | ||
|
||
if ($this->_action != CRM_Core_Action::DELETE && (!$this->_id || !CRM_Core_DAO::getFieldValue('CRM_Core_BAO_MailSettings', $this->_id, 'activity_status_id'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a very long line, 165 characters with whitespace. Might be more readable to break the conditionals into variables.
Would it make sense to use an API call here to fetch the settings? I've been encouraged to use it over BAO calls by some people.
CRM/Admin/Form/MailSettings.php
Outdated
$defaults = parent::setDefaultValues(); | ||
|
||
if ($this->_action != CRM_Core_Action::DELETE && (!$this->_id || !CRM_Core_DAO::getFieldValue('CRM_Core_BAO_MailSettings', $this->_id, 'activity_status_id'))) { | ||
$defaults['activity_status_id'] = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this in the code in a few places, but it really would be much nicer to have constants here instead of magic numbers. Might be too much to ask to change it in this PR though.
CRM/Report/Form/Activity.php
Outdated
INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} ON {$this->_aliases['civicrm_activity_contact']}.activity_id = {$this->_aliases['civicrm_activity']}.id | ||
AND {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$sourceID} | ||
LEFT JOIN civicrm_contact contact_civireport ON contact_civireport.id = {$this->_aliases['civicrm_activity_contact']}.contact_id | ||
WHERE (1) AND {$this->_aclWhere} {$groupByFromSelect} {$this->_having} {$this->_orderBy} {$this->_limit}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the WHERE (1)
?
CRM/Report/Form/Activity.php
Outdated
$sql = "{$this->_select} | ||
FROM civireport_activity_temp_target tar | ||
{$groupByFromSelect} {$this->_having} {$this->_orderBy} {$this->_limit}"; | ||
FROM civireport_activity_temp_target tar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not so clear to me from the description why this query needs to be changed, maybe you could add something to the PR description here or in the ticket
{literal} | ||
<script type="text/javascript"> | ||
CRM.$(function($) { | ||
var $form = $('form.{/literal}{$form.formClass}{literal}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so good with JS, but will this function be applied to new items in the list? I just remember sometimes having issues where functions that are called on state change were not called if the element was added after page load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it works in both add & edit.
@mickadoo thanks for the review. I've taken your suggestions and changed the field to store a name instead of an id, and removed the existing hardcoded Also there were some stray unrelated changes (CRM/Report/Form/Activity.php) that accidentally snuck into this PR. I've rebased them out. This should be good to go. Can you test it on your system if you haven't already done so to verify it correctly sets the activity status of inbound emails? |
if ($this->_action != CRM_Core_Action::DELETE && | ||
(!$this->_id || !CRM_Core_DAO::getFieldValue('CRM_Core_BAO_MailSettings', $this->_id, 'activity_status')) | ||
) { | ||
$defaults['activity_status'] = 'Completed'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check if I understand this conditional:
IF (this is not a deletion) AND ((this is a creation) OR (there is no default value for activity status)) THEN status should be "Completed"
Does that sound right?
@colemanw Looks good to me. About testing locally, I'm really not so familiar with using core CiviCRM, except what I see of it through CiviHR. To my great shame I don't even know how to test your changes locally. I have a container I was using to try run the core tests and from there I ran As regards the review, it's fine for me. If you'd like me to test locally I might need some help getting set up though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine, thanks for splitting it