Closed dmonllao closed 10 years ago
@danpoltawski may I reference you as reviewer?
come on guys, you will not often have the pleasure to review code using jquery :P
The issue description seems simple, the code is massive ;-)
I preferred to add a client-side confirm before submiting a delete action and I included jquery as probably in future we will need more JS to manage other stuff
Hi @rajeshtaneja are you in the moot for a quick review?
The patch affects the web application to view the results, apart from that it only adds a couple of Java lines to the BeanShell script to change the permissions the files are created with. As it is not affecting the tool backend nor the calculated results, base processes... I will merge it myself in a couple of days.
Hello David,
Patch looks good, although I am not sure if delete option should be provided without any login/log. I do understand nightly is not exposed on web, but I think it would be nice to not allow deletion of logs without any authorization. Haven't tested the code yet, will post more feedback after testing it.
Thanks for the review Rajesh; as it is commented on the README, this tool is not secure at all outside internal networks, the consequences of deleting a run results are not necessarily "less bad" than logging into the moodle site using the hardcoded admin password we provide :) I'm not specially keen on create an ACL system only for this feature, but I agree with you that, in case somebody wants to make the web interface public, we could add a setting for it that we could reuse for other future stuff.
Yeah, perhaps some $readonly master switch could, automatically disable any write operation. That would allow safe-publish of results and so on. +1 for that in another issue.
Thanks for fixing this David,
Patch looks spot-on. +1 to get this in.
Thanks Rajesh, changes merged and pushed (BTW: I will stop referencing issues in the commit message, too much noise)
Hello David,
Can you please include properties_read in detect_big_differences.php as it failed performance comparison on nightly01:
Class 'properties_reader' not found in moodle-performance-comparison/webapp/classes/report.php on line 351 moodle-performance-comparison/detect_big_differences.php:0 moodle-performance-comparison/detect_big_differences.php:49
Thanks Rajesh, done and pushed.
Add an option to delete runs (from runs/) from the web user interface. We will need to chmod runs/* or create them with 777 as we don't know about the user system.
Also will be good to be able to download runs quickly (without having to get into the FS) to share with others and/or attach to issues (renaming the file to avoid remote but possible problems would be an extra)