AWeber Webform

Hi,

your description both here and on your project page is very short.
Can you please provide some more context so we/users can understand what is going on. What is Aweber, what does this module do? etcetera.
maybe a screenshot to get a feel for it.
cheers

Don't forget your status or you'll be left behind!

Is there an aweber expert or someone familiar with aweber that can step in please?

Hi,
sorry for the delay, I don't have the time at the moment. I'm bumping this to the top, I'm hoping someone will step in.
The obvious (it might help):
did you install the coder module and check if your module meets the coding standards?

cheers

Code has no documentation.

Also, there's a bunch of other weird stuff in this code.

  if (!db_table_exists('aweber_webform')) {
    aweber_webform_install();
  }

What? Please review hook_install() documentation.

function refresh() {

Namespace ALL your module's functions.

 drupal_install_schema('aweber_webform');

No need for this.

Also, run through Coder *again* please and fix your indentation throughout.

> Added more thorough commenting to the code itself

Please review the standards on code documentation. This is not correct:

/**
 * Help menu block
 */
function aweber_webform_help($path, $arg) {

Your indentation is still wrong in places. Two spaces, no tabs.

// $Id$: aweber_webform.install, v1

No need for this.

function aweber_webform_install() {
    db_query("TRUNCATE TABLE {aweber_webform}");

The table will be empty on install. There is no need to do this.

require_once('aweber_api/aweber_api.php');
require_once('aweber_webform.php');

It would be better to only include these when needed. Perhaps put them in a wrapper funciton aweber_include_api() or something?

    return '<h3>' . t("About") . '</h3><p>' . t("The AWeber Web Form module allows users with an AWeber account to add an")

Please review correct use of t(). Do not break sentences in t() calls and use placeholders.

  switch ($delta) {
  case 'aweber_webform':

No need for a switch with only one case! Also, please check indentation EVERYWHERE.

 * Implements hook_form_validate(). If the submitted for was the authorization

Incorrect on two counts -- this is not a hook. And first line of docs should be ONE line, followed by a blank line. Please review the guidelines again.

I'm not able to try the module as I have PHP 5.2 on my localhost -- so I'm just reading the code. I'd say that what you're doing with forms looks really weird and complicated, but as I'm not sure what the final output is meant to be, I'm not sure how to suggest you improve it.

Aye ignore Coder on that as it has not been updated for git.

> Also, you had suggested that we namespace our methods and we had made that change.

By namespacing, I didn't mean PHP namespacing. Namespacing, used in the Drupal world, means just calling all your functions mymodule_functionname. Sorry for the confusion.

As I said above, a lot of this module's code is just *weird*.

I'll cover a few small things first:

function aweber_webform_install() {
  db_query("INSERT INTO {aweber_webform} (consumer_key, consumer_secret, access_key, access_secret, timestamp) VALUES ('', '', '', '', '0')");

I don't understand why you're putting a sort of blank row into your table on install. What's the point?
Also, you're then checking for it in aweber_webform_form() and then putting it in again! Surely if you put it in on install, you know for sure it's there!

  drupal_uninstall_schema('aweber_webform');

Not needed on D7.

More generally, as I said, you're doing some very strange things with Form API that I'm finding hard to see a reason for.

Eg:

  /**
   * Creates the form that prompts for account authorization.
   */
  public static function get_Auth_Key_Form($form, &$form_state) {
    $form['aweber_webform_authkey'] = array(
      '#type'          => 'textarea',
      '#title'         => t('Authorization Key'),
      '#default_value' => t(''),
      '#cols'          => 30,
      '#rows'          => 2,
      '#maxlength'     => 200,
      '#description'   => t(''),
      '#required'      => TRUE,
      '#suffix'        => '&nbsp;<a href="' . self::AUTH_URL . self::APP_ID .
        '" target="_blank">Click here to obtain an authorization key and copy it into the box above.</a><br>',
    );

    $form['submit2'] = self::submitButtonFake();

    return system_settings_form($form);
  }

Why put this in a class? A MODULE.admin.inc file would do nicely.
What is submitButtonFake?
Do you realize that system_settings_form($form) will cause a variable_set() with all form keys? You don't seem to be using system variables at all, so that is surely superfluous.
Is the point of this a multiform, or could you simply have the authentication form at one admin tab and the settings form at another?

> * If the submitted form was the authorization form, it will try to
* authorize the account and store their data. If it

Validation handles should not usually change anything, only check user input is valid. And you *definitely* don't want a system_settings_form() in here!

In general, I'd like to see this look like Drupal rather than a foreign system mashed into Drupal, if you see what I mean.

I'm also seeing a lot of missing t() calls, though I'd suggest working on untangling the form stuff before fixing that.

> What is MODULE.admin.inc?

Look at Drupal core and how its modules put page callbacks for admin forms in files with that name pattern. This is a pretty basic pattern in Drupal, so I would have expected you to know it.

> This way, when the 'refresh' button is pressed, the text on it is changed to 'refreshing,' and the Drupal form state thinks the hidden button was pressed.

That doesn't sound good at all. Look at how core handles upload buttons that require the user to wait.

> In all examples of Drupal projects, that is the return statement at the end of form and form_validate that we saw.

Drupal core only calls that from form builders: http://api.drupal.org/api/drupal/modules--system--system.module/function...

system_settings_form() is used only for specific form types. What it does is use the keys of form elements to set system variables with variable_set() that can later be retrieved with variable_get(). so, the point is that if you're not using variable_get() to retrieve those variables then you are unnecessarily setting variables. There are other ways to add submit buttons.

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

function mymodule_form_name_submit($form, &$form_state) {

}

Look at system_settings_form() to see how the function works. I think it's in form.inc or system.inc? Anyways, it uses a foreach loop on form elements and sets variables with variable_set() using element keys. In this case it is setting a variable with something that ultimately looks like variable_set('aweber_webform_authkey', $form_state['values']['aweber_webform_authkey']);.

You can just return $form in your form function instead.

http://api.drupal.org/api/drupal/modules--system--system.module/function...

system_settings_form() is a helper for admin settings forms. it does two things:

- adds standard buttons for saving & resetting
- adds a submit handler which saves $form['foo'] as variable 'foo'.

You only need it on the form builder, not anywhere else. Now whether you need it at all is up to you. If you are saving variables, then you probably do.

> Unfortunately, we have not developed any Drupal modules in the past, so we're walking a little blind when it comes to the Drupal community's standards.

A fair amount of immersion is needed to absorb some of the idioms. What worked for me when I was new to Drupal was finding something in core or popular contrib modules that does something similar to what I want to do, and looking at how it works.

> we are having a very difficult time identifying the actual standards being applied to this approval process.

If the documentation isn't up to scratch... file bugs! :)

> In form_validate, we have changed the return to just $form, and that works fine

form_validate should not return anything!

Read the FormAPI guide: http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... If anything in that is unclear -- file a bug! If you never read that before because it wasn't prominently linked from where you did look -- file a bug!

(EDIT: I meant this one: http://drupal.org/node/751826 -- far more useful.)

> We were unable to find an example of a button with changing text. We wanted to have the buttons acknowledge that the action was taking place despite a slow browser/server/what have you. Is there an example of this done in a better way, and if so, where specifically can we find it?

What you want is exactly what I mentioned: the upload file button in core's upload module, or in filefield. The button does not change but you get a spinner beside it. Using the same UI pattern keeps things consistent and furthermore you can probably reuse core's code, or at least the graphic and the css.

I've filed a bug: #1189516: system_settings_form does not really explain what it does.

Hi,

it's obvious to me that you're working hard on this project and want to get it approved. It's taking a very long time and I can see you are indeed in the dark as to the process of getting it approved.
I am too :)
I guess the aweber form is not everybody's piece of cake.

You guys are the domain experts on aweber obviously, so there should be no problem in the code there.
You've been on top of things and have been handling the feedback very well.
As for the drupal code, I cannot imagine that it is not up to par by now and that you are pretty up to date as to what is expected.
If there is anything out of order with the logic/code it will show up with user feedback and in your continuing development effort.

I have full confidence in your work.
Since I did not review I won't change the status, but I think you should get project status by now.

Can someone who reviewed change the status?

> You guys are the domain experts on aweber obviously, so there should be no problem in the code there.

I would agree with this, but that is also the problem.

The applicant(s) are experts in their system, but don't know Drupal at all, and getting them to actually *use* Drupal rather than just bash their system in is proving to be an uphill struggle.

Example: I said to use a .admin.inc file. I can see one has been added. Does it look like a Drupal .admin.inc file? Not in the slightest.

A quick trip round some of Drupal core would show you how to use this. Look at http://api.drupal.org/api/drupal/modules--node--node.module/function/nod... for instance. Look at http://api.drupal.org/api/drupal/modules--node--node.admin.inc/6. The .admin.inc file contains the callbacks for admin pages. It's not a class. It doesn't get loaded with a require_once(), because hook_menu() takes care of this.

This is slowly getting better, but it's still is not Drupal. You guys need to read some more code and pick up the idiom.

Hi guys,

any progress on this?

cheers,

Rolf Vreijdenberger

The application has been marked for need work in 19 weeks, and there is no email address available for the applicant. Marks this as closed (won't fix). @aweber_communications, if this is wrong, please reopen this application.

Is there any chance in getting this up to par? I'd love to help since I'll be doing a project in which this will be used.

This reminds me of my grade school learning to write. Penmanship, the teacher insisting on things being exactly like she wanted. Never mind that the words were clearly ledge able. "No not like that, that's sloppy. You need more home-work practice lazy sloppy work will not be allowed" She said.

Aweber is probably the most reputable name in the email deliverability industry. As such email coming from their servers is not blocked or sent down some black hole. We all use shared servers and networks that are not as diligent about the issue. There is much to be said about that issue.

The reviewer is like that teacher, go dig around the core and other modules to see how we do it. This is a corporate mentality, they actually pay their programmers to develop a module for Drupal users. It was a remarkable thing to see how this issue was handled. It would only make Drupal more desirable to online marking companies who depend on their emails getting through.

To me it looks like the reviewers made the wrong call and should have let this go to project so the users could give their input.

The company would have seen some real evidence that their work was indeed not wasted. It would have been a win win after the community got their hands on it. Although there may not be anyone using Drupal because of this integration or lack of it. I know I am one of those who was sorely disappointed.

I seldom put my 2 cents in but for what it's worth, there you have it. IF the module they submitted passed the tests, it should have been allowed. From there things would take their due course but. the powers that be would not even give them a go at it.

By the way, you said dig around the core and find out how we do it. Did you look at their company and dig around to see what they were all about? Fair's fair.

Anyway, they are paid and we are just volunteers, got to go now. Thanks to everyone for the great program called Drupal, I love it.

Check out this: http://www.aweber.com/blog/new-features/aweber-drupal-module.htm