iCopyright, Inc. is a third-party service that offers article reuse and monetization services on content. This service is currently used by big publishers like SmartMoney, Advanstar, Penton, Investors Business Daily, and the Globe & Mail.

This module automates the process of signing up for the iCopyright service (which is free), and adding the article tools (Print, Email, Post, and Republish) to specified nodes. It enables every Drupal site to enjoy the same content tools that the big boys do without any configuration headaches.

http://info.icopyright.com/drupal-temp has some basic documentation.

Comments

Jonathan Peterson’s picture

Jonathan Peterson commented
Status: Active » Needs review

Obviously the sandbox link would be helpful! Oops.

http://drupal.org/sandbox/JonathanPeterson/1181726

Putting into "needs review", too, as that seems to be the norm.

ccardea’s picture

ccardea commented

Hello Jonathan,

Thank you for your application. What version of Drupal does your module work with?

The wait time for a review is currently approaching seven weeks. You may be able to shorten your wait by contributing to the code review process. All it takes is basic module writing skills. Please see http://groups.drupal.org/code-review for more information on how to participate.

Jonathan Peterson’s picture

Jonathan Peterson commented

It's Drupal 6.

I have in fact started contributing code reviews today in the hopes of getting some good review karma. My goal is to get three issues closed as "reviewed and tested" by end of next week. (Unfounded newbie optimism? We'll see...)

Jonathan Peterson’s picture

Jonathan Peterson commented

FYI I've done a quick screencast to help the reviewer see how it works: http://www.youtube.com/watch?v=yBSalapCDpw

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Do you work for the icopyright service? Not that I care, just trying to figure this out. It looks like the service provides the functionality associated with the little print/license buttons I see on some of the customer websites. Is that correct? And, this module is for integrating that tool into a Drupal install? Would someone be able to test this without an account with the service?

Jonathan Peterson’s picture

Jonathan Peterson commented

Yes, iCopyright sponsored the development of the module. It does, in fact, provide the functionality for the little print/license buttons. The module integrates the tool into a Drupal install; basically it adds the button on the appropriate nodes, and provides a "feed" view of the content for the iCopyright service to pull when it needs to.

You can sign up for an account using the "sign up" tab in the admin section. It's free -- no credit card, no trial period, or anything like that; use a fake name and a fake email address if you like.

The screencast shown linked to above walks you through it.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Here are few things. Sorry for the terseness. I'm typing this up rather quickly.

Use 2 spaces for indenting, not 4. Files are missing @file blocks. Missing drupal core version from .info file (won't install without this), and remove the $Id line. Please comment the constant definitions, and add a hook_help() function.

Instances of drupal_set_message() in the icopyright_admin_settings_submit() function are missing t(). In hook_menu(), descriptions do not need t(). See http://drupal.org/node/140311

For the .admin.inc file, it should be added to the menu using the 'file' value. http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo... . The .module file is always included, even when your module is not operating, so that file is always included (it is not in a function). That would defeat the purpose (saving resources) of separating it out. Same with the common.inc file.

You should be able to use hook_form_FORM_ID_alter() since you are only editing one form. It's up to you, just pointing it out. http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo...

Also, coder reported several issues. Some trivial, some not so trivial. Please review the coder results. Make sure it is set to 'minor' so it will return the most results, and you check off all of the review criteria. (except the converting ones.)

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs review » Needs work
Jonathan Peterson’s picture

Jonathan Peterson commented
Status: Needs work » Needs review

Thanks for your review, David.

First, I've really expanded the README.txt file to include a section on background and orientation. Hopefully this can help you figure your way around the code and save you some time in the review. I know you're volunteering so I want to be as considerate as I can of that.

Also I've made the requested changes:

  • Added @file blocks
  • Removed all $Id: $ lines
  • Implemented hook_help
  • Modified all indentation to match standard
  • Fix t() calls per review
  • Ran through coder at minor level and fixed all but $Id: $ tag
  • Include .admin.inc through file not explicit include
  • Reworked hook_form per suggestion

One thing I did not do was remove the explicit include for icopyright-common.inc. That file includes functions needed from both admin and module for all their operation -- seems to me the file-from-menu-block strategy doesn't work there. If I'm still wrong then please do educate me. :-)

Thanks!

Jonathan Peterson’s picture

Jonathan Peterson commented

Are there any other changes that need to be made prior to acceptance? Anything I can do to help this move forward?

Thanks!

ccardea’s picture

ccardea commented

Don't make any more posts until someone looks at your project, since it bumps your application to the front of the queue, and a lot of reviewers look at the oldest applications first. The ideal situation for you would be for David Hernandez to follow up, since he did the review. You might want to politely let him know directly that you made the changes he suggested, and that your application is ready for a follow-up review.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Looking it over now. Most of the changes look good. I'll try to finish this up very soon. (i.e. this weekend)

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

I get these errors when trying to sign up:

* user warning: CURL emulation does not implement CURL option CURLOPT_FOLLOWLOCATION in C:\wamp\www\drupal-6.22\sites\all\modules\curl\libcurlemu-1.0.4\libcurlnative.inc.php on line 328.
* warning: Invalid argument supplied for foreach() in C:\wamp\www\drupal-6.22\sites\all\modules\curl\libcurlemu-1.0.4\libcurlnative.inc.php on line 368.
* Your submission was rejected by the iCopyright servers.

The errors are from curl, so it _may_ not be your issue, but just double check the usage. I used dummy data for signing up, so I'm sure I did something to cause the rejection, but you might want to notify the user why it is being rejected. (just a suggestion).

Coder indeed found zero errors.

Still looking over.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

It would appear you do not have a hook_help(). Please add one. It is not a requirement, but it will make everyone happy.

Nothing else is jumping out at me, without going a lot further into the nitty gritty. If anything pops-up, I'm sure you'll fix it. All your code has been thoroughly commented, and adheres to the coding standard. You've been attentive and ameable. I'm sure you'll do fine as a maintainer.

Jonathan Peterson’s picture

Jonathan Peterson commented

Thanks for your service David.

hook_help implemented and pushed out.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs work

I was 90% through reviewing with no issues, until I got to the .tpl.php files.

Is there a reason to hardcode all of the styles? These should be done with CSS.

The check_plain() and other PHP function calls in theme/icopyright-feed.tpl.php should be implemented in template_preprocess_icopyright_feed().

JS scripts should be added with drupal_add_js().

Assigning this to myself, to make sure I follow up quickly once you get to this.

Jonathan Peterson’s picture

Jonathan Peterson commented
Status: Needs work » Reviewed & tested by the community

Thanks for the careful review, Tim.

No good reason for the styles to be inline, really -- sloppy on my part. I added some class definitions and created icopyright.css. I call drupal_add_css to load it from hook_nodeapi, when the node is actually getting the tools attached.

I actually hadn't thought of a template preprocess function, but you're right, that's a better place for it. I implemented template_preprocess_icopyright_feed() per suggestion, and cleaned up icopyright-feed.tpl.php so there is no logic in it. Nice.

Regarding drupal_add_js, that I did not do. This is for two reasons: first, drupal_add_js doesn't support external javascript files in Drupal 6; and second, the external javascript in question is designed to be loaded at the place of insertion. I took a close look at the JS in question and conferred over the phone with its author, and we concluded there's no easy way to rework it into two parts (a definition/loader for the head and a DOM inserter for the body). Tim, if you feel very strongly about this, I can probably figure out a way to rework it -- but to this humble coder the effort/benefit ratio just seems really high. Let me know what you think.

Thanks.

(Setting this to RTBC again instead of Needs Review so I don't end up at the end of the queue again -- hope that's not bypassing procedure.)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs review

The purpose of me assigning it to myself was so I wouldn't lose track of you :)

I will double check now, but I think you're probably right, and you might be done!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: tim.plunkett » Unassigned
Status: Needs review » Fixed

I'm impressed, that was a great turn around on your part.

I've granted you full project creation rights. Use this ability carefully!

Thank you for your patience.

Jonathan Peterson’s picture

Jonathan Peterson commented

Hooray! Tim, Dave, thanks so much for your efforts. They are very much appreciated.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Good call, Tim. I'm sure I just glanced over that. And good retort, Jonathan.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.