TMGMT OHT

welcome,

Good to see that you've fixed all issues of the automated report

Please read about the workflow for project applications.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation. Your readme may follow the contents of your project page.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

added link to PAReview results.

you forgot the tag

added info about reviews

Checked through ventral - the code is clear of any errors

Nice work! The code looks great.

A bunch of things that I noticed below, mostly just some ideas/remarks, the only things that should be fixed is the finished() call and the check_plain(). Happy to RTBC this after that.

    $job = tmgmt_job_load(check_plain($_POST['custom0']));

The check_plain() here doesn't make much sense. That should only be used for things that are going to be displayed. You should be able to simply remove it.

    $job->addMessage('Status for project @project changed to "@status". Estimated finish: @finish',

Hint: You *could* use %status instead of adding "", that would wrap it in < em > tags by default. Also

@project_id

in the submitted message.

exit('');

You shouldn't use exit() in a page callback, that can have weird results because some cleanup things might not be run properly. If you don't return anything in your page callback then Drupal will render an empty page by default. If you really want to exit, you should call drupal_exit().

- I noticed that you have a hardcoded supported language list. This is fine but the downside is that when you add a new language on your side, you need to do a new release so that users can translate into that language. The Supertext translator plugin AFAIK does this mapping on the server side. This is absolutely nothing that you need to fix, just something to keep in mind as a possible improvement for a later version.

    $job->finished('The translation has been received.');

You shouldn't do this. Adding translations will automatically set all job items to needs review and once they are accepted (manually or automatically), will mark the job as finished automatically.

   foreach ($data as $key => $value) {
      $items .= str_replace(array('@key', '@text'), array($key, $value['#text']), '<item key="@key"><text type="text/html"><![CDATA[@text]]></text></item>');

Do you have a way to pass a label of the text forward? We're still improving the API part of that, but there is both a #label and a #parent_labels ( I think, in the latest -dev) property that you could use.

Uh, no idea why the "PAReview: review bonus" was removed, does that happen automatically?

It looks like you added a "d" somewhere to a comment, search for "Handling response.d".

Other than that, this looks good to me. Thanks for the quick response.

Any idea already on how to deal with the label? My suggestion would be to open an issue so that it can be discussed there. Please tell me the issue number so that I can follow.

added one more link of manual review

I don't mean translating the labels, just passing them through. See http://drupal.org/node/1645244. They might contain relevant information for a translator.

The advantage of relying on TMGMT is that you don't need to care if something is content or something else. A job could just as well contain comments/commerce products/... through entity translation, or things like menus or taxonomy terms through i18n string translation. That's another reason the labels are important, as they would tell you there if it's a menu link title or description and so on.

manual review:

• tmgmt_oht_callback(): this menu callback is unprotected. Anyone can send POST data to your site which is inserted for translation review. I'm not familiar with the TMGMT architecture here, and the other modules in TMGMT core seem to have the same weakness. Could you explain how a site is protected against random spam translation submitted by someone impersonating the translation service? Maybe I'm missing something here, can't find the access check.

Otherwise looks ready to me.

Oh yes, I missed that, sorry.

Most translators solve this by passing the/some authentication information from the server to the client, e.g. the same api and signed key. Also, I think all translators don't actually submit the translations directly, they just tell the client that the translation is ready and provide an API to fetch translations.

This has two advantages:
a) It's not possible to inject something. the client always needs to connect to the service and authenticate there to get the information.
b) Websites behind a firewall (e.g. intranets, although those probably don't require translations, but dev/staging environments are often not available from the outside) can do a manual fetch, triggered by a button in the checkoutInfo() method, see the nativy translator for an example of that.

I however think that this part is optional and would possibly require a quite major overhaul of your API. However, doing some kind of authentication is quite important.

You have some minor coding standards errors you should take care about:
http://ventral.org/pareview/httpgitdrupalorgsandboxonehourtranslation161...

Else, this is RTBC for me, one question though - is your user account a company or a personal one?

Removing review bonus, please do more reviews to get a quicker approval.

How about the user account, is it personal or company?

Reason for the question: AFAIK it's not permitted to have shared accounts that are used by multiple people, although I currently can't find where this is stated. If that's not the case anyway, maybe rename your account to avoid such confusions.

Note that once your project is approved, you can add as many personal accounts from your company as maintainers for the project as you want and they can have full control over it, the only thing they can't do is create new full projects on their own.

This is stated at http://drupal.org/user/register (you need to log out to access that page)

@DavidS: You still have to answer the question #21, so it is not unclear.

It should be clear but I world also like if you answer the question so we know, so I ask again is your user account personal?

manual review:
tmgmt_oht_callback(): i can still inject random status change messages for a job. I just have to guess a job id. Please make sure that those messages can only be added from an authorized source.

Changes look good to me.

To avoid any discussions: Yes, a call with a wrong argument is going to lead to a watchdog entry, but there are easy ways to do this with a bare Drupal core installation as well, just go to /doesnotexist and it will do the same. So really not a problem.

#21 have never been answered, please do so.

I don't understand what you're trying to achieve here.

#26 is quite clear to me, he states that it is a personal account:

I have changed the name of the account, and I hoped it's clear now that this is personal account.

Translates to Yes, this is a personal account.

As klausi mentioned, this rule is displayed on the user register form so he already agreed to it when he created the account. And even if it weren't so, nobody would honestly answer No to that question in here, knowing that it's not allowed. Which means that your question is pointless, it's enough to clearly state that it must be personal, what has been done multiple times.

Berdir, please do not get into discussion if this is valid question or not here. Raise the question the code review group or on #drupal-codereview. It is a simple question and has not been direct answered by the applicant. I will approve @DavidS as git vetted user after I get a direct answer on that question. (ups, can not do that, I reviewed this one, sorry)

Watchdog is fine, that's what I was trying to say :)

Thanks for your contribution, DavidS!

I updated your account to let you 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 and #drupal-i18n. So, come hang out and stay involved! Thanks again, and good luck! :)