Problem/Motivation

Currently after doing a bulk upload, users need to click "Edit" on each item to assign field data to each one. This is not so bad, but in some cases it can be a problem. Our site uses organic groups, and users can't edit a file that is not in one of their groups, but because the field editing page is bypassed, the file never goes into one of their groups.

Others may just prefer that all the field edits be done on a single page, for less clicking.

Another use case is when file types have required fields, like alt text for images.

Proposed resolution

Write a patch that opens the multiform file edit page after a bulk upload.

Remaining tasks

Review patch.

User interface changes

A new page during the bulk file uploading process where the field data is set.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brockfanning created an issue. See original summary.

brockfanning’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
3.12 KB
siramsay’s picture

thanks for this @brockfanning, works perfectly. bit beyond my understanding of media workings at present so helpful in more than one way.

I used Multiform 7.x-1.x not 7.x-1.x-dev with no problems there, so no need to use dev of that module for this use case.

to your question. Also decide if this should be configurable on/off,

  1. This should just be on as adding ALTs to images should be required in most cases so now that is enforced. (there are some cases where they may not be needed but in bulk upload I would guess ALTs should be required, however that is a debate for another day).
  2. on file/add page of Media Bulk Upload after clicking next it takes you to the admin/content/file/edit-multiple/XXX XXX page , this is not configurable
brockfanning’s picture

Issue summary: View changes

Cool @size. You make some good points, description edited.

Proteo’s picture

Status: Needs review » Reviewed & tested by the community

@brockfanning many thanks for the patch, working perfectly with:

Media 7.x-2.0-beta2
Multiform 7.x-1.1
Plupload 7.x-1.7

And patches for:

Media: here.
Multiform: here.

byronveale’s picture

I can also confirm this is working, presently with media-7.x-2.0-beta5, multiform-7.x-1.1 with patch, and plupload-7.x-1.7.

joseph.olstad’s picture

Status: Reviewed & tested by the community » Fixed

Ok, committed to 7.x-2.x dev but have not yet tagged a release for it.

brockfanning’s picture

@joseph.oldstad: I'd be a little hesitant to commit this patch yet, since it depends on another patch for a separate project. I don't know if anyone has tested it without the multiform patch. (And even if it doesn't break anything, this patch wouldn't be functional until the multiform patch gets committed.)

joseph.olstad’s picture

can someone please follow up with RTBC patch here: bug maintainers to commit it
#1889652: Possibility to hook into multiple edit form

As for the other patch, it was committed to 'media' a while back.

byronveale’s picture

joseph.olstad’s picture

joseph.olstad’s picture

To make this functionality work you will need the latest release of multiform 7.x-1.2 released a few minutes ago.

brockfanning’s picture

Great work @joseph.olstad, thank you!

joseph.olstad’s picture

Thanks @brockfanning, it was mostly your great work that made this possible. Really leaning on your patches. Spoken too soon, see regression :P

joseph.olstad’s picture

Priority: Minor » Major
Status: Fixed » Needs work

This commit caused a regression with plupload integration.

marking this as major priority. Lets redesign this so that it DOESN'T break plupload compatibility.

see related issue:
#2828728: plupload - Pop-up window doesn't close itself and file uploaded are showed inside

brockfanning’s picture

Yikes. Strange though, because I also use Plupload and it has been working fine with the patch in #2. I haven't updated our production setup since the recent commits to media, media_ckeditor, and multiform though, so maybe there is some bad interaction going on. I hope to have some time to look into this later in the week, and update our media setup.

siramsay’s picture

@brockfanning had asked if this should be a toggle-able on/off option but I made these arguments taken from above

to your question. Also decide if this should be configurable on/off,

  1. This should just be on as adding ALTs to images should be required in most cases so now that is enforced. (there are some cases where they may not be needed but in bulk upload I would guess ALTs should be required, however that is a debate for another day).
  2. on file/add page of Media Bulk Upload after clicking next it takes you to the admin/content/file/edit-multiple/XXX XXX page , this is not configurable

So the issue as per #2828728: plupload - Pop-up window doesn't close itself and file uploaded are showed inside is "by design", however , obviously we need to make this toggle-able as some use case don't need to bulk edit after upload.

Later in the #2828728 thread @interdruper says "The same in my case. Moreover the files are not saved to the File field after closing the last window, so the issue is serious" , i am am using the same set up listed with latest production version of Media and files are saved fine, so possible that is another unrelated issue.

Solution, add this as an option on the admin/config/media/browser page. I guess would need to add a new section/form to that page specific to this functionality.

Proteo’s picture

Hey, just wanted to give my two cents. I've been using the latest beta (8) and Plupload in production in a couple of pretty busy sites, along with other sites in different stages of development without a hitch. The only "problem" is that the bulk editing form comes up without proper padding (just like in the screenshot) but that's a theming problem I guess (we use Adminimal).

siramsay’s picture

sorry unsure how to format and submit a patch, but this works below to solve the problem.
I set the new variable to true as feels this a good feature.

will look at rolling proper patch tomorrow



+++/includes/media.browser.inc

  // If we just did a multiple upload, do the multiform file edit. The flag that
  // tells us that we need to do this is $params['render_multi_edit_form'].
  
 +// first check bulk edit is on
 +if (variable_get('bulk_upload_edit', TRUE)) {

  $multi_edit_form['buttons']['cancel']['#access'] = FALSE;
  return $multi_edit_form;
  }
+}


+++/includes/media.admin.inc

'#description' => t('CSS property opacity; used with overlay.'),
  );
  
 + $form['bulk_upload'] = array(
 +   '#type' => 'fieldset',
 +   '#title' => t('Bulk Upload'),
 +   '#collapsible' => TRUE,
 +   '#collapsed' => TRUE,
 + );
  
  +$form['bulk_upload']['bulk_upload_edit'] = array(
  +  '#type' => 'checkbox',
  +  '#title' => t("Bulk upload edit"),
  +  '#description' => t("be re-directed to an edit page after bulk upload on a node edit page"),
  +  '#default_value' => variable_get('bulk_upload_edit', TRUE),
  +);


+++media.install

  variable_del('media_opacity');

+ // bulk edit option
+ variable_del('bulk_upload_edit');
}

joseph.olstad’s picture

@size thanks for your analysis and code.

I'm not sure though, should the default value be TRUE ? Is it safer to be defaulted to FALSE?

Status: Needs review » Needs work

The last submitted patch, 21: bulk_field_editting_after_bulk_uploading-2818683-21.patch, failed testing.

joseph.olstad’s picture

hmm, something not right in the patch.

@size
in order to create a patch, its quite simple, there's a few ways to do it.

this is the standard way:

git clone --branch 7.x-2.x size@git.drupal.org:project/media.git
cd media

now edit the files you want, make the changes, save the files.

git diff > bulk_field_editting_after_bulk_uploading-2818683-23.patch

if you add a new file that didn't exist previously and you want that part of a patch, its slightly trickier, you'll have to do a search on doing that, I could explain it here but its widely documented on the web.

once you've created the new patch, upload it.

If you're stuck on windows and you don't know how to get these tools, simply download 'gitbash' , its a command line tool that has all these commands built in.

joseph.olstad’s picture

@size
to apply your patch, you can do it two ways, if you have the .git repo info you can use
git apply patch_name.patch

If you do not have the .git repo you can use the 'patch' utility

cd media
#download patch to media folder, I use wget

wget https://www.drupal.org/files/issues/bulk_field_editting_after_bulk_uploading-2818683-21.patch
do a dry-run test, does not actually apply the patch
patch -p1 --dry-run < bulk_field_editting_after_bulk_uploading-2818683-21.patch

if dry run works, do this:
patch -p1 < bulk_field_editting_after_bulk_uploading-2818683-21.patch

to reverse the patch, simply add the -R flag

dry-run in reverse
patch -p1 --dry-run -R < bulk_field_editting_after_bulk_uploading-2818683-21.patch
reverse patch for real
patch -p1 -R < bulk_field_editting_after_bulk_uploading-2818683-21.patch

siramsay’s picture

@joseph.olstad thanks for the info, seem all pretty straight forward as I am familiar with GIT.

default true or false is debatable but as stated "on file/add page of Media Bulk Upload after clicking next it takes you to the admin/content/file/edit-multiple/XXX XXX page , this is not configurable " I always felt that it was missing from the bulk upload and when this was bought up and @brockfanning created this patch and then you commited thought it was the right direction.

I guess on install/enable we could return a configuration screen or a least a message with a link to the config page so as people know it is an option. some may also argue it should be configurable per media browser field.

I open to either way as can always toggle on or off site wide and look at ways to make it better in the future on as needed and time allows schedule.

will see if I can trouble shoot patch test fails and re-submit but info is limited on why it is failing.

joseph.olstad’s picture

I'm trusting your advice on the true or false on this one.

Any ideas on the 6 fails for the patch?

joseph.olstad’s picture


testFilesBrowserSort

fail: [Other] Line 564 of sites/all/modules/media/tests/media.test:


testFilesBrowserLibrary

fail: [Other] Line 592 of sites/all/modules/media/tests/media.test:


testFilesBrowserMyFiles

fail: [Other] Line 840 of sites/all/modules/media/tests/media.test:


testBrowserSettings

fail: [Other] Line 705 of sites/all/modules/media/tests/media.test:

testMediaBrowserHooks

fail: [Other] Line 496 of sites/all/modules/media/tests/media.test:

testMediaBrowserWebTab

fail: [Other] Line 114 of sites/all/modules/media/modules/media_internet/tests/media_internet.test:

brockfanning’s picture

Since this is a change in behavior (an edit screen where there previously was none) it might make more sense to default to FALSE. I think that the addition of this feature is the reason for #2828728: plupload - Pop-up window doesn't close itself and file uploaded are showed inside. It's not actually a regression - just an unwanted feature.

We should probably prefix the variable with "media_" so that it's clear what module the variable belongs to - like "media_bulk_upload_edit".

I also believe there is a syntax error in the patch in this line:

if (variable_get('bulk_upload_edit', TRUE), isset($params['render_multi_edit_form']) && isset($params['fid']) && module_exists('media_bulk_upload')) {

(the comma should be &&)

joseph.olstad’s picture

Thanks @brockfanning
this is the new patch with those two changes made,

fixed the syntax error (, instead of &&)

and renamed bulk_upload_edit variable to media_bulk_upload_edit

  • joseph.olstad committed bb9b4f9 on 7.x-2.x authored by size
    Issue #2818683 by brockfanning, size: Bulk field editing after bulk...
joseph.olstad’s picture

Status: Needs review » Fixed

fixed in dev branch , beta11 will be comming soon, maybe tomorrow or a few days from now.

siramsay’s picture

thanks everyone, seems like a small syntax error can throw tests out and sensible suggestion on variable naming too.

as per my suggestion it does appear on install a configure link is present in Media module.info file
maybe we could add it to media bulk upload? this only works on install/enable however but I guess notes will be included with the next release too.

configure = admin/config/media/browser
joseph.olstad’s picture

@size , is this what you're suggesting? (SEE PATCH)

siramsay’s picture

@joseph.olstad yes, I think that would be a good addition to the whole patch

Status: Fixed » Closed (fixed)

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

epersonae2’s picture

Is this actually fixed? Looks like it left off with some patches but nothing in even a dev version?

brockfanning’s picture

I think everything made it in, but you do need to have the latest version of Multiform in order for it to work.

joseph.olstad’s picture

added last minor change to this issue,

diff --git a/modules/media_bulk_upload/media_bulk_upload.info b/modules/media_bulk_upload/media_bulk_upload.info
index 47b6463..f0e8969 100644
--- a/modules/media_bulk_upload/media_bulk_upload.info
+++ b/modules/media_bulk_upload/media_bulk_upload.info
@@ -3,6 +3,8 @@ description = Adds support for uploading multiple files at a time.
 package = Media
 core = 7.x
 
+configure = admin/config/media/browser
+
 dependencies[] = media
 dependencies[] = multiform
 dependencies[] = plupload
joseph.olstad’s picture

Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

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