AutoUpload is a user interface (UI) enhancement for uploading files.

Currently, users must select files, then press the "Upload" button. We found users often don't realize a button press is necessary and mistakenly think their image is uploaded when it's not.

This module removes the extra button press and hides the "Upload" button via JavaScript for a quicker, automatic file upload. When JavaScript is not enabled, it falls back to the "Upload" button.

Project Page

http://drupal.org/sandbox/cristinawithout/1843158

GIT

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/cristinawithout/1843158.git autoupload

Drupal Version
Drupal 6 and Drupal 7

Reviews of other projects
http://drupal.org/node/1536250#comment-6317966
http://drupal.org/node/1086416#comment-6753042, http://drupal.org/node/1086416#comment-6866646
http://drupal.org/node/1536544#comment-6870184

CommentFileSizeAuthor
#10 coder-results.txt1.17 KBklausi

Comments

anwar_max’s picture

anwar_max
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus +PAreview: single application approval, +PAReview: admin mentoring

Hi cristinawithout,

Everything is looking good to me but this is a small module so we cannot give you git vetted user role we can promote this module as a single project.

I am going to remove review bonus tag you can add more review bonus tag after doing the code review of other project applications.

cwithout’s picture

cwithout commented
Status: Reviewed & tested by the community » Needs review

Oops. This is embarrassing. I thought I'd merged my local branch into HEAD, but I didn't. There's quite a bit more code in there now.

anwar_max, would you mind taking a look again and letting me know whether the amount of coding is sufficient to warrant an application to git vetted user? If so, I'll add the review bonus tag back and remove the single project promote tag.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs review » Needs work

Hi,
I will jump straight to the issues:

  • It probably doesn't matter much but it might be better to use a more appropriate key for the fieldset at autoupload.admin.inc:17. It could just be me but predefined doesn't make much sense.
  • Instead of storing whole configuration arrays in variables in autoupload_admin_form_submit(), it would be a better idea to just store the keys. Apart from saving space, if one of the values changes later due to some upgrades, the code will continue to work properly. With the current method, the variables will continue to hold outdated values unless user goes to the config form and resubmits.
  • autoupload.install:12 - Consider using module_load_include, if relevant.
  • Consider using $.once (see the last section in Managing Javascript in Drupal 7) as I can see the style tag added multiple times in the head. Also, with this in mind, maybe you can figure out a way to not use style at all.
  • autoupload.js:46 onwards - Consider using switch block instead of if .. else if .. else.
anwar_max’s picture

anwar_max
Primary language English
commented
Status: Needs work » Needs review

Hi Christinawithout,

Manual Review:

Everything is looking good to me. I don't think that you can get GIT vetted access because this module still has less code but I will check with klausi and will let you know.

Automated Review:

Result of automated code review.

Review of the 7.x-1.x branch:

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.

@hussainweb :
You cannot change the status of this ticket as mentioned here as cristinawithout is following all the coding standards and drupal best practices and we cannot force her to follow what we are thinking we can only suggest her.

anwar_max’s picture

anwar_max
Primary language English
commented
Status: Needs review » Needs work

sorry to change the status.

hussainweb’s picture

hussainweb
Primary language English
commented

@anwar_max, @cristinawithout,

Sorry about that. I am not sure if my suggestions fall (formally) into Drupal best practices but I certainly do think that they would fall in general best practices. Of course, I don't mean to hold up an application for that. I thought the status was meant to reflect suggestions too.

Thanks for the link. I will keep it saved to make sure that only those steps are needed for the status change. All others are suggestions but while not changing the flag. I think these suggestions would then go into the project issue queue, right?

cwithout’s picture

cwithout commented
Status: Needs work » Needs review

Thanks for the review, hussainweb.

  • The variable name is what it is. A list of the predefined field types and whether or not they're enabled. It will make more sense when future planned functionality is added. The "predefined" variable is just a boolean indicator of which predefined configurations are enabled. Future plans are to allow users to enter their own types of fields and all the necessary data to do so (via config input and/or hooks).
  • All the configuration data is stored to keep the JS as simple as possible and for speed at runtime. The plan is to just use a hook_update() if any changes are ever needed. It's a little more to update code-wise than building the configuration data in hook_init() to pass to the JS, but I made the choice to do all the processing on the admin side rather than at run time for speed's sake. If I implement a hook for other modules to add their own definitions, I'll probably build those at runtime so the maintainers don't have to worry about handling updates.
  • module_load_include() isn't relevant in this case. It's used to load files from other modules. I'm loading a file from my own module where require_once is preferred over module_load_include().
  • I would love to not need to use the style at all, but there is no way I've found. I initially used hide() and show(), but they don't work in these instances, because the button being hidden is sometimes (as the case with the Media module) added dynamically after the code has run, and the adding of the button doesn't trigger a Drupal behavior or have an event to act upon. I didn't know Drupal core had the once plugin. I made some changes so only one style tag is appended.
  • Good call. I should have made that a switch in the first place.

Thanks, anwar_max.

I ran the automated tester before submitting the module. Those results are false positives. The automated reviewer appears to be assuming that / and + are binary operators and should have spaces before and after. In this case, they're not. They're part of a RegEx. Spacing before and after would ruin the regular expressions.

hussainweb’s picture

hussainweb
Primary language English
commented

Hi,

Thank you for the clarifications. I think it all looks good. Regarding storing the config data, I would still prefer storing just the key but I see you have already planned for it.

I understood the false positive for the automated tester and agree, and that is why it was ignored in earlier reviews, I think.

I think it is ready to go ahead. As @anwar_max said, I am not sure about the code size limit. Everything else looks good.

Thanks again for an useful contribution. I am wondering if this will work together with File Sources and File Source Plupload integration.

anwar_max’s picture

anwar_max
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: single application approval

Hi Cristinawithout,

Everything is looking good to me except some issue I can here but this is not a roadblock and this is ready to go and we can give you GIT vetted role as mentioned here.

Removing "PAReview: Single project promote" tag.

Thanks for your contribution :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new coder-results.txt1.17 KB

Review of the 7.x-1.x branch:

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.

manual review:

  1. "scripts[] = js/autoupload.js": why do you need the JS on every single page request? Shouldn't you only add the script on pages where one can actually upload files?
  2. autoupload_update_711(): make sure to follow the update function naming pattern next time, i.e. you should use 4 digits. See http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...
  3. autoupload_init(): I think you should implement a field hook instead and add the JS + settings there?

But that are not hard blockers, so ...

Thanks for your contribution, cristinawithout!

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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

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