CVS edit link for Manonline

I developed a civicrm module (CiviCRM Phone Campaigns) that creates "campaign phone calls" activities, when I create a Phone Campaign, for each target contact in the campaign.

Actually, there are only mail campaigns in civicrm (Mailings), but sometimes we need to do special recovery campaigns like one-to-one marketing or relational marketing instead of massive campaigns. For this is CiviCRM Phone Campaigns.

Comments

Manonline’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new2.17 KB

Oh, the project includes a patch to apply to the CiviCRM core, because the hook_civicrm_post wasn't called. Here is the bug report: http://issues.civicrm.org/jira/browse/CRM-6271.

The patch add the line who calls the hook.

avpaderno’s picture

Status: Needs review » Needs work
Issue tags: +Module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module/theme; for modules it should include also a comparison with the existing solutions, while for themes a screenshot is also required.

Manonline’s picture

First sorry for my english, I'll try to explain it with my basic English :)

There aren't an other module with this functionality or similar.

When a company wants to recover the contact with one or more clients, the best way isn't a mailing, because you must maintain a relationship with them. Or you just want to do a direct marketing campaign to a specific sector with a major responsiveness.
With CiviCRM you can do a recovery campaign via mail using de CiviMail Component, but it isn't the best way to resume the contact with an old client.
You can create an activity with all contacts you want, but you can't track each contact activity.
This module provides two activity types: "Phone Campaign" and "Campaign Phone Call". The Phone Campaign is the header activity of the campaign and you must create it with all target contacts you want to recover contact. This will create an activity (Campaign Phone Call) for each contact, letting you to track them individually.

Basically, CiviCRM Phone Campaigns provides a way to do personal recovery campaigns or direct marketing campaigns.

I have plans to provide a block listings campaigns and their statuses. Probably a dashlet to place it in the CiviCRM dashboard.

avpaderno’s picture

Status: Needs work » Needs review

Thanks for the reply.

Manonline’s picture

First sorry for my english, I'll try to explain it with my basic English :)

There aren't an other module with this functionality or similar.

When a company wants to recover the contact with one or more clients, the best way isn't a mailing, because you must maintain a relationship with them. Or you just want to do a direct marketing campaign to a specific sector with a major responsiveness.
With CiviCRM you can do a recovery campaign via mail using de CiviMail Component, but it isn't the best way to resume the contact with an old client.
You can create an activity with all contacts you want, but you can't track each contact activity.
This module provides two activity types: "Phone Campaign" and "Campaign Phone Call". The Phone Campaign is the header activity of the campaign and you must create it with all target contacts you want to recover contact. This will create an activity (Campaign Phone Call) for each contact, letting you to track them individually.

Basically, CiviCRM Phone Campaigns provides a way to do personal recovery campaigns or direct marketing campaigns.

I have plans to provide a block listings campaigns and their statuses. Probably a dashlet to place it in the CiviCRM dashboard.

Manonline’s picture

there are news about my cvs account application?

thanks :)

Manonline’s picture

pinging :)

dagmar’s picture

Status: Needs review » Needs work

Some comments about your module:

You need to follow the Coding Standards

  if(!$atid['header']) {

Needs a space between if and (.

    $params = array(
                'label' => 'Phone Campaign',
                'description' => 'Create Phone Campaigns for contacts.',
                'weight' => '10',
                'is_active' => '1',
                'is_reserved' => '1',
              );

For arrays you should use two spaces: http://drupal.org/coding-standards#array

    $params = array(
       'label' => 'Phone Campaign',
       'description' => 'Create Phone Campaigns for contacts.',
       'weight' => '10',
       'is_active' => '1',
       'is_reserved' => '1',
    );

And probably (I'm not sure, but probably yes...) you have to enclose the strings, label and description, into t().

Don't include the version in the info file

version = 1.0

And related to the patch attached. You should post this patch in the CiviCRM module explaining why it should be committed.

Manonline’s picture

Status: Needs work » Needs review
StatusFileSize
new2.16 KB

Thanks for your comments.

Here is the reviewed module with the correct indentation and spaces.

I had created a sub-task in the CiviCRM's issue tracker for http://issues.civicrm.org/jira/browse/CRM-6271.

changing status to need review

Manonline’s picture

the patch is now approved and it will be included in next releases.

Manonline’s picture

any update?

I made a new module, a dbFM (Data Base File Manager) mapper (dbfm_mapper) for Feeds, and I want to contribute with that module too.

dagmar’s picture

Status: Needs review » Needs work

There are still some issue with the Coding Standars

All headers for functions like:

/*
 * Implementation of hook_schema()
 */

Should be:

/**
 * Implementation of hook_schema().
 */
$otid = db_result(db_query('SELECT activity_type_id FROM civicrm_activity WHERE id=%d', $objectId));

Should enclose civicrm_activity in { } and id=%d should have spaces

$otid = db_result(db_query('SELECT activity_type_id FROM {civicrm_activity} WHERE id = %d', $objectId));

This code:

            $calls[] = array(
                          'id' => $row['pcid'],
                          'activity_name' => 'Campaign Phone Call',
                        );

Should be

            $calls[] = array(
               'id' => $row['pcid'],
                'activity_name' => 'Campaign Phone Call',
            );

Related to:

    require_once 'api/v2/Activity.php';

This is wrong. There is not such archive into your module, and therefore will never be included. Take a look to http://api.drupal.org/api/function/module_load_include or provide some documentation about how this file have to be included.

It would be nice to see a bit more of documentation about what this module does. Comments do not hurts :)

Also, the .info file is corrupted, I cannot review it, please attach it again.

If you speak Spanish, you can join to the #drupal-es IRC channel to get some feedback about how to apply to your cvs account faster.

Cheers!

avpaderno’s picture

Actually, a hook implementation comment should be

/**
* Implements hook_schema().
*/
Manonline’s picture

Status: Needs work » Needs review
StatusFileSize
new2.16 KB

Thanks for replying me.

I had corrected errors mentioned above. The code is attached.

About this

    require_once 'api/v2/Activity.php';

Note that before require_once, there is a civicrm_initialize(), this puts me into the civicrm context. If I don't call that function, I cant require the API file without the full path.

Here is a code example extracted from CiviCRM's Wiki (http://wiki.civicrm.org/confluence/display/CRMDOC32/Using+CiviCRM+APIs+-...)

This snippet uses the v2 (new-style) Event Search API. The example code will run as-is in a Drupal block which has format set to PHP code. Note that the specific v2 API - Event.php is included up front.

if (module_exists('civicrm')) {
    civicrm_initialize( );
    require_once 'api/v2/Event.php';
    $params = array ( 'event_type_id' => 3,
                      'is_public'     => 1
                      );

    // Search API returns a nested array
    $myEvents = civicrm_event_search( $params );

    // Uncomment this line if you want to see all array items returned from your search

    if ($myEvents) {
        foreach ( $myEvents as $event ) {
            $display = $event['title'] . '<br />Starts: ' .
                $event['start_date'] . '<br />Ends: ' .
                $event['end_date'] . '<br /><hr>';
            echo $display;
        }
    } else {
        echo 'No events found.';
    }
}

Sorry for the poor documentation, I will work on that (I need to take some English classes hehehe).

avpaderno’s picture

Status: Needs review » Needs work
  1.             $calls[] = array(
                  'id' => $row['pcid'],
                  'activity_name' => 'Campaign Phone Call',
                );
    
    

    If the string is shown in the user interface, it should be passed to t().

  2.             else {
                  db_query('DELETE FROM {civicrm_phonecampaign} WHERE pcid = %d', $params['id']);
                  watchdog('civicrm', t('Error deleting campaign phone call with ID !pcid. Error: @error', array('!pcid' => $params['id'], '@error' => $result['error_message'])));
                }
    
    

    The call to watchdog() is not correct.

  3. Remove any debugging code, also the commented out one.
Manonline’s picture

StatusFileSize
new2.09 KB

Thanks again for your time.

  1. Yes, it's now passwd by t(). Also I passed by t() the first part of the activity subject:
                $params = array(
                  'activity_type_id' => $atid['detail'],
                  'source_contact_id' => $source_contact_id,
                  'assignee_contact_id' => $assignee_contact_id,
                  'target_contact_id' => $tcid,
                  'subject' => t('Phone Call from campaign') .' '. . $objectId . ' - ' . $subject,
                  'details' => $details,
                  'status_id' => $status_id,
                  'activity_date_time' => $activity_date_time
                );
    
  2. Oh, that watchdog call isn't supposed to be there. Removed.
  3. Debugging code has been removed from the module.
Manonline’s picture

StatusFileSize
new2.07 KB

Oh, I double concatenate the string. Fixed here.

Manonline’s picture

Status: Needs work » Needs review
brianV’s picture

Status: Needs review » Needs work

A few notes:

  1. In line 127 of the .module, 'true' should be 'TRUE' (Booleans are uppercased.)
  2. Messages to watchdog() should actually not be wrapped in t().
  3. On that note, your watchdog() calls have improper syntax. For instance,
    watchdog('civicrm_phonecampaign', t("Error deleting campaign phone call with ID !pcid. Error: @error", array('!pcid' => $params['id'], '@error' => $result['error_message'])), WATCHDOG_ERROR);
    

    should be:

    watchdog('civicrm_phonecampaign', "Error deleting campaign phone call with ID !pcid. Error: @error", array('!pcid' => $params['id'], '@error' => $result['error_message']), WATCHDOG_ERROR);
    

    To clarify, watchdog() takes both $message and $variables paramenters, which behave exactly like the $string and $args parameter you would pass to t().

  4. Both the .install and .module file are missing @file docblocks. See http://drupal.org/node/1354#files
  5. Hope this helps.

zzolo’s picture

Component: Miscellaneous » miscellaneous
Status: Needs work » Closed (won't fix)

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

  • The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
  • Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.
avpaderno’s picture

Component: miscellaneous » new project application
Issue summary: View changes