Weblab: files api unescaping and asset manager delete fix

[cpirich]

• AssetManager's deleteAssetRow() had a bug where it reset assets based on the return value of assetListStore.remove() - which ignored the extension filtering that normally happens inside assetListStore.list(). So now deleteAssetRow() follows the pattern of onUploadDone(): it modifies assetListStore, then notifies that the assets have changed, then it updates the assets by calling assetListStore.list() with the allowedExtensions parameter.
• The files API was occasionally storing escaped filenames inside the manifest.json file (e.g. {"filename":"puppy%20dog.jpeg"}). We now standardize on storing unescaped filenames (e.g. {"filename":"puppy dog.jpeg"}) and whenever we make explicit comparisons with entries in the manifest, we also unescape the filename we are comparing.

[islemaster]

Overall approach looks good. It looks like we need new unit tests to go along with the files_api.rb changes. I'm also not sure whether the test_property_limits_enforced failure on Circle is caused by your changes, but it's at least tangentially related so worth checking out.

[islemaster]

Nit: Probably not very expensive, but since we're always going to call CGI.unescape in this method we might as well only call it once and reuse the result - then we won't be doing it in a loop here. Likewise with the downcase variant which shows up in a couple of loops.

[cpirich]

Updated

[cpirich]

Added unit test for escaping.

[islemaster]

Codecov reports none of this code has been exercised, even though you just wrote a unit test. I suspected that coverage reporting for shared tests is broken, so I went ahead and pulled the branch to see if I could easily make this test fail by reversing your API changes.

It was harder to break than I expected, so I'm a little concerned that your new behavior isn't covered.

• All of test_files.rb still passes if I delete all but one use of downcase from the files_put_file method (the downcase call on line 486 seems to be the one that's required to pass). test_case_insensitivity fails if I remove that last use of downcase.

• All of test_files.rb still passes if I delete all uses of CGI.unescape from the files_put_file but use the existing VCR fixtures. If I run without VCR test_escaping_insensitivity still passes if I remove the CGI.unescape call at line 507.

So I still think coverage might be broken since I was able to break the tests with some changes to these methods (@pcardune any ideas?), but I also think there are quite a few uncovered branches here that we should revisit. Let's not block the bugfix going in on getting this figured out, but do it as a follow-up instead.

[cpirich]

Thanks for looking in to this. One possible case that is probably not tested is the rename operation where the old name could be unescaped, though in practice that is hard to do since it is a query parameter.

However, the bigger issue that I had is that our test helper method get_object doesn't like unescaped paths (probably at the Rack::Test::Methods level). So the tests for now verify that creating files with either unescaped or escaped paths will both work equally well - and can be retrieved and deleted using escaped paths.

[pcardune]

I don't think tests in shared contribute to coverage reports at the moment, only tests in dashboard. So the coverage we are seeing here is probably from what dashboard tests are generating.