[D7] ToDoist API

Please add hook_uninstall and add variable_del for "todoist_api_user_token" variable.

HI @manav,

Awesome module. Integrates very well with Todoist.

Manual Review:

* Code Looks Good.
* Coder & https://pareview.sh/node/949 returned no issues.
* Installed and tested its functionality, works great.

Extra Suggestions:
* Add a documentation to look for "API Token" in todoist.com, which is under "todoist settings > Account > API token".
* Also, if you could change the description of API field at /admin/config/user-interface/todoistto be more specific from "Add Todoist user token" to "Add Todoist user API token"

Rest looks good to me. Thanks for contribution!

Hi Manav ,

Nice Module and Working fine for me.

My findings are :

All user facing text should be passed through t().
Examples are :

      $header = array(
        array('data' => 'Task', 'sort' => 'asc'),
        array('data' => 'Task added date'),
        array('data' => 'Task due date'),
        array('data' => 'Action', 'colspan' => 3),
      );

!empty($node['date_string']) ? $node['date_string'] : 'No due date',

'empty' => 'No task created...',

And your hook_help data also.

Dear @manav,

re-tested the module, Looks good to me. Suggestions in #10 also implemented.

Thanks!

Hi Manav,

Still i can see : hook_help data is not passed through t()

$output .= '<p>This module intregate todoist api functionality in drupal
    project. Using this module user can access their todoist.com account basic
    functionality in drupal project. They can add, edit, delete and complete
    their task. They will get remender mail time to time sent by
    todoist.com.</p>';
    $output .= '<ul><li><b>On Todoist website</b>
    <ul><li>First you have to signup or login on Todoist.com.</li>
    <li>After sign-up/login on Todoist official website, please go to
    user account section and here you have to create an api key.
    <p>Settings > Account > API token</p></li></ul>
    </li>
    <li><b>On your Drupal website</b>
    <ul><li>Download this module under sites/all/module and enable it.</li>
    <li>Copy the API key (which you have created on Todoist official website)
    and paste it to your Drupal website under Todoist configuration setting.
    <p>Administration » User interface » Todoist configuration</p></li>
    <li>Now you get all your task, which you created pervious on Todoist
    official website or you can able to edit/delete task here
    <p>Administration » Content » Todoist</p> You can also add new task here.
    Every time when you create new task or edited any task, this will save
    on your Todoist.com account automatically.</li></ul>
    </li>
    </ul>';

Hello Manav,

I have found some issues with application:

1. When I install module. It's show me following warning message:

User warning: The following modules is missing from the file system: todoist. For information about how to fix this, see the documentation page. in _drupal_trigger_error_with_delayed_logging() (line 1128 of C:\xampp\htdocs\drupal\includes\bootstrap.inc).

2. There are no title with configuration form and when we submit form without any data it does not display any error message. Please see attached image.

3. Please use define() for API URL or you can get this information from configuration form.

define('API_URL', "https://todoist.com/API/v7/sync?");

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Module is working fine. Moving it to RTBC.

Manual Review

Filename: todoist_api.admin.inc

/**
 * Implements hook_form().
 */

It's not a proper comment. Refer here for all hooks information.

'#description' => t('Get your ToDoist API token from
      <a href="@token">todoist.com</a>.',
      array("@token" => "https://todoist.com/")),
    

You can use placeholders and l() function to achieve this, instead of using anchor tags. Like:

'#description' => t('Get your ToDoist API token from !todoist.', array(
      "!todoist" => l('todoist', "https://todoist.com/")
    )),

This can be improved at several places in module.

$form['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Save'),
  );

/**
 * Implements Submit handler for todoist_config_form().
 */
function todoist_api_config_form_submit($form, $form_state) {
  if ($form_state['values']['todoist_api_user_token']) {
    variable_set('todoist_api_user_token', $form_state['values']['todoist_api_user_token']);
    drupal_set_message(t('Your configuration has been saved.'));
    $form_state['rebuild'] = TRUE;
  }
}

You can use return system_settings_form($form); instead of writing an additional submit handler until and unless you are trying to achieve something else. I assume here that you are only trying to save variable value from form.

if (!isset($form_state['values']['todoist_api_user_token']) || $form_state['values']['todoist_api_user_token'] == '') {

You can !empty() to achieve the same. Please refer here.

form_set_error('Token', t('Please enter ToDoist Api Token value.'));
form_set_error expects first parameter to be the name of form element, it should be: form_set_error('todoist_api_user_token', t('Please enter ToDoist Api Token value.'));.
Also you have used #required which will automatically do the end required task, else condition of validation will be just fine!

$http_result->status_message !== 'OK'
Suggestion: !=

$user_apicall = TODOIST_API;
  $user_apicall .= 'token=' . $token;
  $http_result = drupal_http_request($user_apicall);
  

You are using these more than once, I would recommend to make it as helper function which accepts $token as parameter. Or, if its possible use it only at the time of validation and not in module file :-) as you have already verified the token.

Filename: todoist.module

if (!($http_result->code == 0)) {
Suggestion: if ($http_result->code != 0)

Filename: todoist_api.inc

$form['cancel'] = array(
    '#type' => 'submit',
    '#value' => t('Cancel'),
  );

Cancel button will never act as cancel button. See attached screenshot.

$ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, $apicall);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_POST, 1);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $post_data);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
    curl_exec($ch);

Can be moved to a helper function.

Status was changed to RTBC by Manoj while I was writing the reviews . So changed it again to Needs Work!

Forgot to add the screenshot, my bad.

Clear to go now :-), major blockers are resolved.

Review of the 7.x-1.x branch (commit c1c0bcf):

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

1: Update the message
drupal_set_message(t('1 task successfully edit.'));

like : 1 task updated successfully.

2: I can see the commit message are repeating
example :

Commit 317a945 on 7.x-1.x
by Manav

update all file with new changes according to the issue comment on project application page.

February 21, 2017 16:47
Commit e290d79 on 7.x-1.x
by Manav

update all file with new changes according to the issue comment on project application page.

February 21, 2017 16:40
Commit 9b82570 on 7.x-1.x
by Manav

update all file with new changes according to the issue comment on project application page.

Please follow the pattern : https://www.drupal.org/node/52287 for commit messages.

other wise all looks good for me.

Thanks for your contribution, Manav!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Assigning Credits.