[D7] Jobvite Careers

Hi,

Just done a quick automated review and your module is passing almost 100% cleanly. The only outstanding issue is that the master branch in your repository still contains code. There are instructions for moving to a version branch here, although you will only need to follow step 5.

Now for a more manual review:

> At the top of hook_menu(). you should declare the $items array. Currently you are pushing onto an array which doesn't exist.

function jobvite_careers_menu() {

  $items = array();

  $items['careers'] = array(...

> Your implementation of hook_perm() is a Drupal 6 hook. This has been replaced by hook_permission() in Drupal 7. In your case it would look something like this:

/**
 * Implements hook_permission().
 */
function jobvite_careers_permission() {
  return array(
    'administer careers xml' => array(
      'title' => t('Set the careers XML file'), 
      'description' => t('Allow users to edit the careers XML file.'),
    ),
  );
}

> Your main page callbacks appear to be called jobvite_careers_careers_settings() and jobvite_careers_position_settings() and the function comment says "Settings for the webform". Is this out of date? These functions aren't settings pages. More suitable names might be jobvite_careers_listing() and jobvite_careers_position()

> Regarding the external feed - perhaps you could consider caching the XML locally. If this module were installed on a high traffic site, there might be a lot of requests going out to jobvite. I don't know if their API is rate limited at all, but when you're using a web service it is a good idea to put measures in place to prevent you hammering it. For example, you could cache the results locally and only refresh them in a Cron hook.

> Could you perhaps put in a comment explaining the use of json_encode and json_decode after you've loaded the XML. I assume the goal is to get yourself a "normal" data structure instead of a SimpleXML object. Is it necessary? Is this the best way to do this? (Feel free to educate me on this one!)

> When users request an invalid position you are currently doing this:

// Invalid position accessed.
if (!$valid) {
  drupal_goto('careers');
}

Personally I would prefer to see drupal_not_found() used here. Technically you have accessed something which doesn't exist. Maybe at least display a message to the user stating that they have requested something that doesn't exist and that they have been redirected if you don't want to go down the 404 route.

> This is a slightly more general thought so feel free to shoot it down, but could your module perhaps make use of the Feeds and Feeds XPath Parser modules. It strikes me that you have an external XML feed, which you want to take and display locally in some way. As a developer, if I were to use your module, I would want to be able to take that external XML data and display it locally myself. E.g. I might want to make my own custom Views block to display the jobs. It might be that this more custom use case is best served by the manually using those 2 modules and that your module is perhaps aimed less at developers and more at people who just want a plug and play solution. Just a thought anyway, feel free to ignore this one.

Could you provide an example jobvite XML URL with which to test this module? I haven't actually installed and tested it yet.

Overally it's looking fairly clean and tidy though. If you could provide a testing URL I will take another look and let you know if I have any other comments once I have it up and running.

Cheers,

Andy

Just a quick further note on the local storage / caching. One upshot would be that you could streamline your code that checks for individual positions. Currently you have this code for finding an individual job:

foreach ($xmlarray['job'] as $job) {
  if ($job['id'] == arg(1)) {
    ....
  }
}

So you are looping over an array and returning the first job which matches the id. Really, this kind of task is the job of the database engine. What if there were 200 jobs in the array? 2000? You might iterate 2000 times before you found the right job which seems inefficient. If you had the jobs stored in their own table you could use SQL to retrieve the correct job e.g and let the database do the job it is good at.

db_query('SELECT * FROM {jobvite_careers} WHERE id=:id', array(':id' => arg(1));

Cool. For reference I have found an XML feed here for Mozilla

http://www.jobvite.com/CompanyJobs/Xml.aspx?c=qpX9Vfwa

I'll give it a whirl later and see what happens.

Sure, I'll take a look at caching the xml locally for you on on saturday, am a bit busy tomorrow...

Hi,

I've had a bash at the local caching of results from jobvite. You can find my sandbox here:

http://drupal.org/sandbox/andymantell/1600286

If you want to bring my work across to your sandbox I think what you need to do is add my respository as another remote and do a git pull from it. Or alternatively you could grant my user access to your sandbox and I could push to it. Your call.

http://drupal.org/project/1600286/git-instructions

As a summary of what I have done though:

• Created a jobvite_careers table (You will need to run update.php to create this).
• Moved all XML fetching and processing to jobvite_careers_cron().
• Created jobvite_careers_load() and jobvite_careers_load_multiple() for use in the 2 menu callbacks.
• jobvite_careers_load() is used as a wildcard loader.
• I have removed the careers.tpl.php template and used a theme_item_list() instead as that is essentially what this template did.
• I have exposed a few more of the job properties in the position template. (E.g. date etc). All of the fields in the source XML are present in the new table.
• I have added a permission called "view careers" as I thought using "access content" was a bit general
• I have tweaked the admin form callback to actually use the "administer careers xml" permission as it wasn't previously in use, leaving it open to all users.

Let me know what you think. When I set out I didn't realise it was going to involve quite so much refactoring, but I think we've ended up with something a bit more robust. It is certainly hugely faster than it was as it no longer has to load that XML every time.

Unfortunately, now that I've been involved, I probably shouldn't be the one to review the code any more!

Cool, I just pushed to your sandbox and it all seems to have gone up fine. Go ahead and test it and make sure you are happy with what I've done! Don't forget to run update.php...

On a side note, I noticed that you haven't set up your git user info to match the settings in your Drupal.org profile though so your user account isn't properly attributed in the commit log (Which means I am listed as the only committer on *your* project!).

You need to set your name and email address in your local config to match those found under "Git Access" when you edit your Drupal.org profile.

Alternatively, add whatever email address you already have set in your local git config to your list of email addresses on Drupal.org. (This would probably be better, as I think your existing commits might get attributed properly then, not sure about this though.)

Cheers,

Andy

There's a page about here about Identifying yourself to Git

Also, your project page could do with some fleshing out. Making it reflect the contents of your README.txt would be sufficient.

This looks good for me, settings this as RTBC.

Thanks for contributing, mmellado!

I know that this process is more so designed for reviewing potential committers' code quality rather than the project itself, however my feedback is specific to this project's suitability for contrib. Though I think the code here is well written, I am personally not in favor of seeing this as a full project, and while my reasons may seem biased, I hope you'll look a bit deeper if this is your reaction.

I maintain several solutions for importing xml data to Drupal: Feeds XPath Parser, Rules XPath Parser, and Views XML Backend. (I recommend the latter.) Each of these can be used for everything this project provides (except for the theming). They are very extensible and have developing user bases with additional modules built on them. There may be some value to having product specific importers, but IMHO, they should be based on one of those projects or a better solution.

mmellando, I want to encourage you to continue contributing while at the same time maintaining my core belief in the need to maintain the long term quality of contrib, so please feel free to question these opinions if you do not share them (yet?). Everyone of the projects in contrib have been made by individuals, groups, companies, etc that offer benefits to the community. I would honestly feel terrible if you felt that I was trying to keep you from gaining more experience and contributing too, so again, please challenge these opinions if you prefer, but at the same time, please also try out those alternative approaches and see if they fit your own development strategies.

This has been sitting idly for two and half months. Is this module abandoned? If so, please change the status.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

It seems this module cannot work on itself, i installed it but it doesn't have any configuration on user interface at all
can someone tell me a step by step guide on how to go about it as the upper information is not cleared enough for me to follow
Please a little help

The last status of this application was not Needs work, so it cannot be closed as Won't fix, since Needs review implies reviewers need to do something, not the applicant. (The lack of activity from reviewers is not a reason to stop an application.)

Since nobody objected, I am going to mark this issue as fixed.

Thank you for your contibution!

I am going to update your account so you can opt into security advisory coverage now.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.