BUG Ensure that file replacement checks for clashes with existing files #370
Conversation
tractorcow
force-pushed
the
pulls/4.0/fix-replace-setname
branch
from
cbfafd2
to
aa35863
Compare
| if (oldExt !== newExt) { | ||
| // name field to autofill if extension has changed | ||
| // If server has requested a filename change, modify it here | ||
| if (json.Name) { |
There was a problem hiding this comment.
This can have the undesired effect of replacing the Name value an edited Name field with the original value? It's not the best user experience.
There was a problem hiding this comment.
How about I conditionally send it back from the server; No json.Name means no change needed.
There was a problem hiding this comment.
I've changed it so that the current Name is always posted back, so that json.Name will reflect the current value, unless explicitly changed.
|
|
||
| // If extension is the same, attempt to re-use existing name | ||
| if (File::get_file_extension($tmpFile['name']) === $file->getExtension()) { | ||
| $tmpFile['name'] = $file->Name; |
There was a problem hiding this comment.
rather than using the Name saved in the database, I wonder if the component could send the API the current value of the field on the form?
…s a property Requirement for silverstripe/silverstripe-asset-admin#370
tractorcow
force-pushed
the
pulls/4.0/fix-replace-setname
branch
from
aa35863
to
4721f33
Compare
|
Updated based on feedback. Note that framework PR silverstripe/silverstripe-framework#6527 is now required for this fix. |
| And I wait for 1 second | ||
| And the "Name" field should contain "file1.jpg" | ||
| When I press the "Save" button | ||
| And I wait for 2 seconds |
There was a problem hiding this comment.
Not too happy about manual waits here, but it's just following a bad precedent. We need to work out if a fetch() is in progress through Behat feature context
There was a problem hiding this comment.
It's annoying I know, but we need a better wait-until-ready solution for behat.
Codecov Report
Continue to review full report at Codecov.
|
|
I can't reproduce the composer issue locally; Downgrading to 1.3.1 to see if it's due to the recent composer release. |
|
I've raised composer/composer#6115 to investigate the possible regression in composer 1.3.2. I suspect that the issue might be due to asset-admin dependency in the PR branch and the one cached by packagist differing. I've raised #378, and suspect that force-merging this (with a broken build) and then updating packagist may resolve the issue. It can't help to experiment. :) |
tractorcow
force-pushed
the
pulls/4.0/fix-replace-setname
branch
from
be78f75
to
6f852fb
Compare
|
I've force-merged #378 and updated packagist to determine if the issue lies between travis and packagist (which could explain why I couldn't resolve the issue locally). |
|
For some reason that issue resolved itself, yet without leaving behind any explanation as to why it was broken. :) |
|
Only failure now is the behat failure, i.e. a real one. However this already exists prior to this PR. (siiigh). |
Fixes #369
Requires silverstripe/silverstripe-framework#6527
Note: Updated so that existing files are detected to avoid duplication, as per discussion with @sminnee