[#KNL-155] CLONE -SAK-800 should be disabled or removed from 2.6.x

Kernel - K1

CLONE -SAK-800 should be disabled or removed from 2.6.x

Created: 15-Apr-2009 19:04 Updated: 05-Mar-2010 02:46

Component/s:

Impl

Affects Version/s:

1.0.5, 1.1.0

Fix Version/s:

1.0.6, 1.1.0

Time Tracking:

Not Specified

Issue Links:

Cloners

This issue cloned from:

~~SAK-16036~~ SAK-800 should be disabled or removed from 2.6.x

Relate

This issue relates to:

SAK-800 Upload, download and extract Zip archive files in Sakai Resources Tool

This issue is related to:

KNL-273 Outstanding issues with SAK-800

1.0.x Status:

Resolved

Description

« Hide

Since there were still so many issues with SAK-800, it's status was recently brought up by Stephen on the QA mailing listlist, Because of followup concerns and based on the comments for on the jira for SAK-800, I believe that it shouldn't be released in the final build. This contributed patch was merged into trunk in an unfinished state and made it into the 2.6 release. Ideally the issues in this patch should be resolved but this will unlikely happen before the release and it should not hold it up.

Here were my comments on the QA list:

- There is no feedback to the user when anything goes bad, as the link appears on resources that you don't have permission to (this could be checked for permission first in FolderCompressAction) and there was an NPE as indicated on the jira.
- No solution for extracting files that already exist in resources
- If the zip file or number of contained files is large, the extraction/compression might take a long time and the UI will block interaction rather than threading it. An alternative would be to have it check sizes in advance. I think I even tested a zip file containing 0K small files and it took almost 5 minutes to uncompress.
- I wasn't sure if compressing large files/directories ran into memory errors or not, some of these edge cases should be tested.
- I don't think I tested what happens operated on really isn't a zip file, just has a .zip extension There's no error messages shown by the tool, so
- More errors in general need to be show to the user.

This should be able to be easily disabled in the UI, you'd just have to wrap up
the actions in FileUploadType/FolderType
actions.put(ResourceToolAction.EXPAND_ZIP_ARCHIVE, new FileUploadExpandAction());

I don't know if we'd want to wrap these up with a user settable sakai.properties value, comment it out of the code or unmerge this patch. It would be nice to eventually see it working. I would probably vote to disable via property just to give someone interest on possibly working on it again. Like resources.zip.experimental = false.

Description

Since there were still so many issues with SAK-800, it's status was recently brought up by Stephen on the QA mailing listlist, Because of followup concerns and based on the comments for on the jira for SAK-800, I believe that it shouldn't be released in the final build. This contributed patch was merged into trunk in an unfinished state and made it into the 2.6 release. Ideally the issues in this patch should be resolved but this will unlikely happen before the release and it should not hold it up. Here were my comments on the QA list: - There is no feedback to the user when anything goes bad, as the link appears on resources that you don't have permission to (this could be checked for permission first in FolderCompressAction) and there was an NPE as indicated on the jira. - No solution for extracting files that already exist in resources - If the zip file or number of contained files is large, the extraction/compression might take a long time and the UI will block interaction rather than threading it. An alternative would be to have it check sizes in advance. I think I even tested a zip file containing 0K small files and it took almost 5 minutes to uncompress. - I wasn't sure if compressing large files/directories ran into memory errors or not, some of these edge cases should be tested. - I don't think I tested what happens operated on really isn't a zip file, just has a .zip extension There's no error messages shown by the tool, so - More errors in general need to be show to the user. This should be able to be easily disabled in the UI, you'd just have to wrap up the actions in FileUploadType/FolderType actions.put(ResourceToolAction.EXPAND_ZIP_ARCHIVE, new FileUploadExpandAction()); I don't know if we'd want to wrap these up with a user settable sakai.properties value, comment it out of the code or unmerge this patch. It would be nice to eventually see it working. I would probably vote to disable via property just to give someone interest on possibly working on it again. Like resources.zip.experimental = false.

Show »

All

Comments

Work Log

Change History

Subversion Commits

git Commits

Sort Order:

[ Permlink | « Hide ]

Anthony Whyte added a comment - 17-Apr-2009 13:33

Ye olde hack:

FolderType.java

// ~~KNL-155~~/SAK-800 Hack; archive file handling is buggy; enable by property setting only
if (ServerConfigurationService.getBoolean(RESOURCES_ZIP_ENABLE,false)) {
actions.put(ResourceToolAction.COMPRESS_ZIP_FOLDER, new FolderCompressAction());
}
FolderUploadType.java

// ~~KNL-155~~/SAK-800 Hack; archive file handling is buggy; enable by property setting only
if (ServerConfigurationService.getBoolean(RESOURCES_ZIP_ENABLE,false)) {
actions.put(ResourceToolAction.EXPAND_ZIP_ARCHIVE, new FileUploadExpandAction());
}

SAK-800 needs to be sorted out so hacks like this can be removed.

[ Permlink | « Hide ]

Anthony Whyte added a comment - 17-Apr-2009 13:39

new property: resources.zip.enable (true/false; default=false). See ~~SAK-16036~~.

[ Permlink | « Hide ]

Anthony Whyte added a comment - 20-Apr-2009 14:53 - edited

I've polled the K1 committers with binding votes (Ian Boston, Stephan Marquard, David Horwitz, Beth Kirschner, Seth Theriault, Ray Davis and myself) and the majority opinion (with two dissents and one did not vote) is to leave in place the property setting I implemented in order to hide the archive compress/decompress option that is added to a hashmap in the kernel (K1) before being passed to the resources tool as options in an action dropdown list.

Yet there exists a good deal of discomfort about this approach amongst those in the Community who have voiced their opinions on the list. I think it important to take account of this advice. So I am going to comment out the property "fix" in kernel /content and remove the property added to /config *.sakai.properties. While I think the property wrapper a workable temporary solution to a challenge we have regarding 2.6.0 the lack of overall Community consensus renders the fix less than ideal in my mind. In addition, I received suggestions that the property be left undocumented in order to further mask the option of enabling the zip compress/decompress feature. I think this is a bad idea. When people start talking about hiding properties implemented to hide code (experimental or challenged) then it's time to jettison the overall idea (and implementation) and opt for a different approach.

Commenting out this code is a temporary solution (as was wrapping it in a property). It is not a replacement for either fixing it or removing it and relocating it to a branch where the issues associated with it can be addressed. Sophisticated users can hack the code themselves although it means they will have to modify both the kernel and Sakai to enable the functionality.