Apache Allura™

Thanks for the contribution. It is a big one, and I'm somewhat busy currently, so might be a little while before we can get back to you with feedback.

Sorry for the big delay in reviewing this. Here's my feedback below. What do you think? Are you still interested/able in making some more updates to this? I can help some (especially python 3) but maybe you could make another set of updates first to do what you can with this feedback? Thanks for beginning this contribution. It's a big chunk of code, and a useful feature for sure.

• nice work adding the license header to the new files. that is important. However, can you make it the very top of the file. ForgeFiles/forgefiles/files_main.py for example has other comments above it.
• for jinja_master/nav_menu.html
  • should have some sort of conditional around it, in case the FilesApp is not installed and active. I see jinja_master/master.html has some JS code to hide the button. It'd be better to do it here server-side instead. Something like {% set files_appconfig = c.project.app_config_by_tool_type('files') %} and {% if files_appconfig %} ...
  • and then instead of /p/{{c.project.shortname}}/files/ having hardcoded p and files parts, you can do {{ files_appconfig.url() }}
  • you shouldn't need to pass ?app_url= in the link. def download_file can determine its own app_url value with c.app.url()
• there should be some permission checks on all the create/edit/delete/etc endpoints (all the ones with @require_post. Otherwise anyone can make changes if they constructed the right requests! Should link_file have @require_post too?
• for security also, I think all the Upload, UploadFolder and UploadFiles queries/updates/deletes should have app_config_id=c.app.config._id added to their query (it could replace project_id if that's already in the query). This field is automatically present via inheriting from Artifact. And querying with it will enforce that the ids being queried/updated/deleted are part of the current app. So nobody can mess around with URLs and try to delete a file id that belongs to some other project.
• in download_file the line M.Mailbox.subscribe(type='direct') seems out of place, is that intended? And instead of setting all response.header values, maybe the serve_file() helper in Allura could be used and simplify things?
• Upload, UploadFolder and UploadFiles should have some indexes specified for the most common fields they are queried on. Like
• Over the past year or so, we've gotten Allura's master branch compatible with Python 3 and runs on py3 by default. So this package needs to work with python 3 too. I've done a lot of that conversion and know what needs to happen, but it could also be good if you are able to get a Python 3 development environment to help with testing that out.

And some code quality suggestions that would be nice, but not necessary:

• M.main_orm_session.flush() can be removed from most or all places. At the end of a web request, there is middleware that flushes everything. So you only need to explicitly flush if there's code immediately afterwards that expects it to be saved. Or sometimes within tests.
• in publish_folder the email sending might be nice refactored as a separate function. And maybe don't even need to loop over user_ids? It seems there should be some core Mailbox or Notification code to handle that, but I don't remember offhand.
• instead of def file_size you could use h.do_filesizeformat
• instead of file.uploaded_by() I think you can do file.author since it's an AlluraUser field.
• is_embedded() and metadata_for() don't seem to be used and could be removed?

And maybe a future separate enhancement - it could be nice for logged-out visitors to browse the folders.

Hello
The above review points have been taken care. The code has been committed to ticket #8368.
As suggested by you, python3 changes and other review points have been committed separately.

Regarding M.Mailbox.subscribe(type='direct'), this feature is for the non-admin users. If an user downloads a file, he is automatically subscribed to the Files plugin of that project. This is the reason why we have added it in download_file .

Please go through the code changes and share your reviews.

Hi @sanskriti123 thanks for your patience on this. I've reviewed your changes and made several commits of my own. You can look at them at https://forge-allura.apache.org/p/allura/git/ci/3f92e1941a9b5c5f6c20aafbfc0a7ba8fab40b1f/log/?path= if you want. I'll probably merge this in a few days, but wanted to give you a chance to look at my updates as well.