API UploadField functions on new records #1862

Merged
merged 2 commits into from May 29, 2013

Conversation

Projects
None yet
9 participants
Contributor

tractorcow commented May 9, 2013

Fixes silverstripe#1827

This update allows the FileUpload field to work on unsaved dataobjects, meaning that creating content with imagery is no longer a two step task.

3.1 upgrading and uploadfield.md documentation have all been updated.

The field now works well as a front-end utility for allowing user file upload management. This should be great news for creators of sites with lots of user generated content. An example has been added to the uploadfield.md documentation.

Some config options no longer make sense due to the way the module's core behaviour has been changed, and have been removed (replaceExistingFile).

I've fixed quite a few functionality, validation, and CSS issues that have probably always been there, or that have been introduced during development.

UploadField::validate actually validates now.

The fileupload-ui javascript library has been extended further to try to develop a more user friendly experience: No more has_one image fields with multiple image upload, better error messages, and better error recovery.

Configuration functions can now be chained.

setConfig and getConfig were pretty poorly documented and not well defined, so they've been made protected and replaced with proper property getters and setters with more intelligent logic, and of course per-method documentation.

Field works well on javascript-disabled browsers. The field works as a regular uploader and properly generates Image objects on form submission.

Fixes the following additional bugs (or at least, the below bugs were not noted during testing):

Not sure if fixed (but probably not):

I think some of the above issues (with has_many Images) were caused by users not correctly extending the Image class with the other end of the relation. This has been better clarified in the documentation.

Contributor

Josua2012 commented May 10, 2013

This is fabulous Damian.
I hope that they will be merged soon.

Thanks,
Regards,
Jose A.

Josua2012 referenced this pull request in symbiote/silverstripe-memberprofiles May 10, 2013

Open

BUG: The avatar field is not working in the profile form (uploadfield). #52

Contributor

tractorcow commented May 15, 2013

Here's a commit I need to merge into this branch:

silverstripe#1940

Contributor

tractorcow commented May 15, 2013

I've rebased and merged in the changes from silverstripe#1940.

Owner

wilr commented May 20, 2013

Impressive as always Damian! I haven't done much with UploadField, @Zauberfisch, you have more experience in this area. Thoughts?

Tested it out myself and works well for DataObject / ModelAdmin which is the usual use case. The 'From Files' option looks funky as on the front end but not may people will enable that option by default. Only other note is I would include the feature as an NEW note in the changelog as well since the changelog file will be what people refer to for 'new' features and if they see this I'm sure you'll encourage a few upgrades.

Happy from my end to merge, the other guys may have some feedback.

Owner

halkyon commented May 20, 2013

Wow, this is awesome. Great work @tractorcow!

Hmm, there's a few API changes here. I'd argue this should go into master as we're trying to stabilise 3.1 for RC.
@chillu, what do you think?

Owner

chillu commented May 20, 2013

Hmm, its a very desireable feature to have, but I think it'll have to be master instead of 3.1 due to the API change. Damian, I haven't looked in detail, but is there a way to get away using the feature without the visibility change of getConfig() and setConfig()? Even if that's the case, we'd need to find volunteers to do extensive testing on those new features until or at the hackathon this weekend in order to consider it for 3.1.

Contributor

tractorcow commented May 20, 2013

I wasn't quite sure how to correctly replace setConfig / getConfig. I feel that it's neither good form nor usable to have a single function interact with many different parts of an API, hence my implementation of individually documented getters and setters.

I suppose that I could make those methods public again, and simply replace each use of setConfig and getConfig with direct access of the $this->ufConfig array, and then add deprecation notices to each. The reason I didn't do so was that it still had a useful purpose to the class itself, hence making it protected.

We can make it 3.2 if you like.

It's kind of an API (changes UploadField functions), BUG (Fixes usability issues) and NEW (introduces new functionality). Is there not a tag that encompasses all of these at once?

Contributor

tractorcow commented May 20, 2013

It was really tough getting my head around the uploadfield javascript API. I don't know why javascript libraries always have to be so complicated! It's like learning a new framework every time I make a change.

@tractorcow tractorcow commented on an outdated diff May 20, 2013

javascript/UploadField.js
@@ -55,7 +55,31 @@
$('.ss-uploadfield-item-edit-all').show();
$('.fileOverview .uploadStatus').addClass("good").removeClass("notice").removeClass("bad");
}
- }
+ },
+ _create: function() {
+ $.blueimpUI.fileupload.prototype._create.call(this);
+ // Ensures that the visibility of the fileupload dialog is set correctly at initialisation
+ this._adjustMaxNumberOfFiles(0);
+ },
+ attach: function(data) {
+ // Handles attachment of already uploaded files, similar to add
+ var self = this,
+ files = data.files,
+ valid = true;
+ $.each(files, function (index, file) {
@tractorcow

tractorcow May 20, 2013

Contributor

Sorry, my indentation in this file is off. Another accindent! Will fix when I rebase and merge.

@tractorcow tractorcow commented on an outdated diff May 20, 2013

javascript/UploadField_downloadtemplate.js
@@ -16,7 +20,7 @@ window.tmpl.cache['ss-uploadfield-downloadtemplate'] = tmpl(
'</label>' +
'{% if (file.error) { %}' +
'<div class="ss-uploadfield-item-actions">' +
- '<div class="ss-uploadfield-item-cancel ss-uploadfield-item-cancelfailed"><button class="icon icon-16">' + ss.i18n._t('UploadField.CANCEL', 'Cancel') + '</button></div>' +
+ '<div class="ss-uploadfield-item-cancel ss-uploadfield-item-cancelfailed delete"><button class="icon icon-16" data-icon="delete" title="' + ss.i18n._t('UploadField.CANCELREMOVE', 'Cancel/Remove') + '">' + ss.i18n._t('UploadField.CANCEL', 'Cancel') + '</button></div>' +
@tractorcow

tractorcow May 20, 2013

Contributor

Inconsistency of translated string here. Title says "cancel/remove". Button text says "cancel"

@tractorcow tractorcow commented on an outdated diff May 20, 2013

javascript/UploadField_uploadtemplate.js
@@ -20,7 +20,7 @@ window.tmpl.cache['ss-uploadfield-uploadtemplate'] = tmpl(
'<div class="ss-uploadfield-item-start start"><button class="icon icon-16" data-icon="navigation">' + ss.i18n._t('UploadField.START', 'Start') + '</button></div>' +
'{% } %}' +
'{% } %}' +
- '<div class="ss-uploadfield-item-cancel cancel"><button data-icon="deleteLight" class="ss-uploadfield-item-cancel" title="' + ss.i18n._t('UploadField.CANCELREMOVE', 'Cancel/Remove') + '">' + ss.i18n._t('UploadField.CANCEL', 'Cancel') + '</button></div>' +
+ '<div class="ss-uploadfield-item-cancel cancel"><button class="icon icon-16" data-icon="minus-circle" title="' + ss.i18n._t('UploadField.CANCELREMOVE', 'Cancel/Remove') + '">' + ss.i18n._t('UploadField.CANCEL', 'Cancel') + '</button></div>' +
@tractorcow

tractorcow May 20, 2013

Contributor

Same here

@chillu chillu commented on the diff May 20, 2013

forms/UploadField.php
- } elseif($record->has_one($name)) {
- $item = $record->{$name}();
- if ($item && $item->exists())
- $this->items = array($record->{$name}());
- }
- }
- $this->items = new ArrayList($this->items);
- // hack to provide $UploadFieldThumbnailURL, $hasRelation and $UploadFieldEditLink in template for each
- // file
- if ($this->items->exists()) {
- foreach ($this->items as $i=>$file) {
- $this->items[$i] = $this->customiseFile($file);
- if(!$file->canView()) unset($this->items[$i]); // Respect model permissions
- }
- }
+ return $this->items ? $this->items : new ArrayList();
@chillu

chillu May 20, 2013

Owner

This assumes $this->items is cached by setValue() before, right? Can we work around this somehow? If not, it needs some PHPDoc to make this dependency clear (and potentially make the method protected?)

@tractorcow

tractorcow May 20, 2013

Contributor

The value of the field and the items are synchronised, yes, and kept update at the same time. This is why setItems calls setValue, and setValue updates the items before calling parent::setValue. The only difference between the two is that items is a list of File objects, where value is the serialisable list of file IDs.

Both setValue and setItems will update both items and value, and $items and $value are already both protected. I'm not sure exactly what needs to be worked around! Maybe I'm just being daft though? Can you elaborate please on what the major concern is? ^^

@tractorcow

tractorcow May 20, 2013

Contributor

Another way this could be implemented is to only store the files in $value and generate dataobjects when getItems is called.

However, whenever this happens it would require additional database hits, so I've limited it to be as query-light as possible. Each file record must be checked anyway whenever the value is changed, so I've just attempted to reduce doubling up of processing effort.

@chillu

chillu May 20, 2013

Owner

I've just looked at getItems() in isolation (typical mistake in code review), but the way you put it makes sense. The field acquires items by having setValue() called (either directly or indirectly), if you call getItems() before this its empty of course. Some PHPDoc on setItems() and setValue() to document this sync will help though.

@tractorcow

tractorcow May 20, 2013

Contributor

Oh, I love PHPDoc! Will do.

@chillu chillu commented on the diff May 20, 2013

forms/UploadField.php
@@ -623,55 +1185,26 @@ public function performReadonlyTransformation() {
}
/**
- * Determines if the underlying record (if any) has a relationship
- * matching the field name. Important for permission control.
- *
- * @return boolean
- */
- public function managesRelation() {
@chillu

chillu May 20, 2013

Owner

Can we deprecate rather than remove?

@tractorcow

tractorcow May 20, 2013

Contributor

I will add it in, but since the refactor the function actually no longer has any purpose.

Is it ok to have a deprecated function that does nothing?

@tractorcow

tractorcow May 20, 2013

Contributor

Also, should we have a deprecation message and THEN an error? It's probably better than a non-breaking warning.

@chillu

chillu May 21, 2013

Owner

Given that the method relies on $record being set on the instance, it seems to be used during the saving process only (and probably should've been protected in the first place). OK, leave it removed, having it empty and deprecated will confuse more than "method not found".

@chillu chillu commented on the diff May 20, 2013

docs/en/reference/uploadfield.md
@@ -42,13 +42,13 @@ based on a has_one relation:
@chillu

chillu May 20, 2013

Owner

Might be worth documenting how file uploads behave when the underlying record is not saved yet. Presumably the field will upload files, but if form saving fails at a later point, those references will be lost and the uploaded files stay on the filesystem. Some scenarios might require to treat uploads more conservatively, and disable this behavour?

@tractorcow

tractorcow May 20, 2013

Contributor

If the form saves but fails due to validation issues, then the value of that field will be retained (which is one of the benefits of properly implementing setValue).

You can try it by setting the file limit size to something tiny and uploading a too-large file. The validation will pop up, but it should stay set against the form until removed and a valid file is added.

In the future we could add some validation failed behaviour, such as to delete all uploaded files and clear them, but I think I'll keep that out of this pull request for the time being.

I'll update the docs to explain how this works.

Contributor

tractorcow commented May 24, 2013

I'm going to make a new pull request, rebased onto master.

Contributor

tractorcow commented May 24, 2013

Please see #1987 for the 3.2 compatible version.

I've also updated the following:

  • Extra documentation on saving / validation behaviour
  • Extra PHPDoc on setItems / setValue
  • Deprecation of getConfig / setConfig
  • Fixing of whitespaces and translation tweaks as noted above.
Owner

sminnee commented May 25, 2013

@tractorcow shall we go with this one over #1987? We'll need to ensure that setConfig() / getConfig() isn't deprecated. Let me know when that change is made; it would be great to get it into 3.1-rc1

Contributor

tractorcow commented May 26, 2013

Hi @sminnee , yes let's go with this one - I'll update it tomorrow, merging in the recent uploadfield changes (something to do with overwriting files?). Apologies for being late on this one, as I'm spending some time with the wife this weekend before I get back to work. :)

Thanks for the feedback earlier!

Contributor

tractorcow commented May 26, 2013

To pull into this - #2009

Doesn't look like it's going to be a trivial merge I'm afraid!

Contributor

tractorcow commented May 26, 2013

Same with 22c7bbf

Contributor

tractorcow commented May 26, 2013

Oh dear, I can't merge in #2009 - There is no longer any attach ajax call (UploadField::attachFile was removed), as it's now a client-side only operation on my pull request. =( The function can't possibly work without being rewritten, if it's still even possible to implement.

@sminnee , can you make a call on what you would prefer? Preparing my pull request for 3.1, or the recent changes / tweaks to the existing functionality, and I will instead merge in my changes to 3.2?

I also had a look at the file replacement warning feature. I see that it puts the list of all uploaded files directly into the field's data - Wouldn't that become a bit unwieldy for folders with lots of files? (I have some clients with a few hundred files) It is also probably worth making a note on front-end use of this field if developers want to avoid exposing their asset folder structure to public view. How would you feel about making this a separate ajax call? I suppose it could also be done as a response via the upload method, but that would probably require quite a bit of work to get working robustly.

Contributor

tractorcow commented May 26, 2013

I'll push on for now and attempt to rewrite the module to support these features. Will let you know if I get stuck at all!

Contributor

tractorcow commented May 26, 2013

After much whinging on my part I've managed to merge in #2009. Removing the serverside code and updating the attach and attachFiles functions in UploadField.js fixed it.

I've also taken the liberty of applying some vertical margin to the buttons to better support multi-line button rows.

Attacking the file replacement feature now.

@tractorcow tractorcow API UploadField functions on new records
Fixed regression from 1e5d404 (UploadField::canPreviewFolder).
Merged in pull request #2009 - (6018bdd).
Merged pull request #1259 (34bfc86).
7f057ce
Contributor

tractorcow commented May 27, 2013

I've rebased and squashed all commits for this, with a couple of recent updates:

  • setConfig / getConfig are alive and well and are no longer deprecated. There is however an error if getting or setting invalid options to enforce proper usage.
  • Merging and reimplementation of this field for compatibility with the pull requests over the last few days. I have re-implemented part of the overwrite-notification method - It now uses an ajax callback rather than relying on a listing of all files stored in the field metadata up front.
  • Updated documentation (most recent updates from my 3.2 version copied back here).

I've left a rebased / un-squashed version of this branch for viewing at https://github.com/tractorcow/sapphire/commits/3.1-uploadfield-enhancement-stash

I also neglected to properly test AssetAdmin in my previous pull request, my apologies for this. It should work fine now.

Contributor

tractorcow commented May 27, 2013

I hear that screenshots are appreciated, so here you go:

image

Contributor

ARNHOE commented May 27, 2013

@tractorcow I am not entirely sure if I should be asking this here. But after you clicked on "Choose another file" wouldn't it be alot easier to have thumbnails, because it will make it easier to select another image.

Contributor

tractorcow commented May 27, 2013

It certainly would be... it's a part of the file selection dialoge. It can be done, I just haven't looked into it yet.

Owner

halkyon commented May 27, 2013

This seems to fix the issues I've raised in #2019 and #2018 as well. Should we pull this in?

Contributor

tractorcow commented May 27, 2013

Is this ok with you @sminnee ?

Contributor

tractorcow commented May 28, 2013

I did also fix #2020 yesterday, sorry I didn't bring it up until now!

Owner

chillu commented May 29, 2013

I've tested this with has_one and has_many on Chrome, in ModelAdmin with validation errors - all works beautifully. The only change I made was adjusting the field width to match the text fields (was 16px too wide).
Added some more stubs to our frameworktest module to streamline manual testing:
silverstripe/silverstripe-frameworktest@2bd2d5f

Merged. EPIC!

@chillu chillu merged commit 163917b into silverstripe:3.1 May 29, 2013

1 check passed

default Scrutinizer: No Comments — Travis: Passed
Details
Contributor

tractorcow commented May 29, 2013

Wow thanks Ingo, this is very exciting. I think it's my biggest pull request that's been accepted so far. :)

Can't wait for the 'other' pull request to be pulled!

Owner

chillu commented May 29, 2013

Yeah, that was a piece of cake compare to the prepared statements one... ;)

tractorcow deleted the tractorcow:3.1-uploadfield-enhancement branch May 29, 2013

Owner

halkyon commented May 29, 2013

Nice @tractorcow, you'll be getting your contributor stats up :)

Contributor

tractorcow commented May 29, 2013

Year, I saw the graph at http://github-dashing.herokuapp.com/default. It's nice to see that being chatty on github gives me imaginary internet points somewhere!

You should have more than 0 commits to any of the 109 monitored repos in May though, right?
There's something wrong with the BigTable query, need to have a look …
Yay for imaginary internet points ;)

On 29/05/2013, at 11:59 PM, Damian Mooyman notifications@github.com wrote:

Year, I saw the graph at http://github-dashing.herokuapp.com/default. It's nice to see that being chatty on github gives me imaginary internet points somewhere!


Reply to this email directly or view it on GitHub.

Contributor

tractorcow commented May 29, 2013

You don't have commits counted against the account who merged them? I'm not actually a staff member, so I'm not allowed to commit, just make pull requests. :)

Yes, that was my suspicion as well, its a similar issue:
The underlying data is normalized in a lossy way, only
retaining the last commit of any given push to github:
https://github.com/igrigorik/githubarchive.org/blob/master/bigquery/transform.rb#L105
https://github.com/chillu/github-dashing/blob/master/lib/big_query_backend.rb#L191
http://developer.github.com/v3/activity/events/types/#pushevent

I think I'll need to fall back to the good old github API for that one again… :/
http://developer.github.com/v3/repos/commits/#list-commits-on-a-repository

On 30/05/2013, at 12:18 AM, Damian Mooyman notifications@github.com wrote:

You don't have commits counted against the account who merged them? I'm not actually a staff member, so I'm not allowed to commit, just make pull requests. :)


Reply to this email directly or view it on GitHub.

Contributor

tazzydemon commented Aug 25, 2013

Is the autosave feature (non 2 step upload) available on 3.1rc1 now. If so, is it default? I note also you speak of extending the image class. In my work I have extended the dataobject class with my OrbitImage class which has has_one relationship with a one. This seems to work fine. Is it wasteful?

Its merely the function:

function getCMSFields() {
$fields = parent::getCMSFields();
$imageField = new UploadField('Image','Choose Images');
$imageField->setFolderName('Uploads/orbit-slider');
$imageField->setRecord($this);

Which I cannot make autosave thus making upload of images (or at the very least moving on to a new entry) a two step affair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment