-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(admin): add organization-level file size and total size limits #18496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add upload_limit and total_size_limit fields to Organization model - Update upload logic to consider organization limits (most generous applies) - Add admin interface for managing organization limits - Enhance project admin to show limit hierarchy with clear visual display - Add links from project admin to organization limit settings - Include comprehensive test coverage for all new functionality Organization limits work alongside project and system limits, with the most generous limit always being applied during file uploads. The project admin interface now clearly shows which limit is in effect and where to modify each limit type. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Resolved conflicts by keeping both sets of changes: - Organization limit management functionality - New role management functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have't gotten through all of the PR yet, but wanted to give some initial actionable feedback findings.
@@ -299,6 +301,14 @@ class Organization(OrganizationMixin, HasEvents, db.Model): | |||
index=True, | |||
comment="Datetime the organization was created.", | |||
) | |||
upload_limit: Mapped[int | None] = mapped_column( | |||
Integer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary, it's achieved by Mapped[int]
Integer, |
and migrations shouldn't change.
CheckConstraint, | ||
Enum, | ||
FetchedValue, | ||
ForeignKey, | ||
Index, | ||
Integer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop if other suggestion taken
Integer, |
size_limits_to_check.append(project.organization.total_size_limit) | ||
|
||
file_size_limit = max(filter(None, limits_to_check)) | ||
project_size_limit = max(filter(None, size_limits_to_check)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: now that there's more than a single source to the limits, would this be better expressed as properties on Project? Something like:
class Project:
...
@property
def file_size_upload_limit(self) -> int:
limits_to_check = [MAX_FILESIZE, self.upload_limit]
if self.organization:
limits_to_check.append(self.organization.upload_limit)
return max(filter(None, limits_to_check)
And then callers can use:
if file_size > project.file_size_upload_limit: ...
A similar property for Project.total_size_limit
would follow a similar approach, with other values.
The advantage is that it moves the ownership of these concepts back to the model, and removes some code from the ever-growing legacy upload function.
if not upload_limit: | ||
upload_limit = None | ||
else: | ||
try: | ||
upload_limit = int(upload_limit) | ||
except ValueError: | ||
raise HTTPBadRequest( | ||
f"Invalid value for upload limit: {upload_limit}, " | ||
f"must be integer or empty string." | ||
) | ||
# The form is in MiB, but the database field is in bytes. | ||
upload_limit *= ONE_MIB | ||
if upload_limit > UPLOAD_LIMIT_CAP: | ||
raise HTTPBadRequest( | ||
f"Upload limit can not be greater than the overall limit of " | ||
f"{UPLOAD_LIMIT_CAP / ONE_MIB}MiB." | ||
) | ||
if upload_limit < MAX_FILESIZE: | ||
raise HTTPBadRequest( | ||
f"Upload limit can not be less than the default limit of " | ||
f"{MAX_FILESIZE / ONE_MIB}MiB." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: now that there's at least 4 views that do limits and validate inputs, maybe externalize to a form class for all of them so they have the same validation rules.
permission=Permissions.AdminOrganizationsWrite, | ||
request_method="POST", | ||
uses_session=True, | ||
require_methods=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no CSRF?
@@ -677,6 +685,110 @@ def organization_application_decline(request): | |||
) | |||
|
|||
|
|||
@view_config( | |||
route_name="admin.organization.set_upload_limit", | |||
permission=Permissions.AdminOrganizationsWrite, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Projects have a AdminProjectsSetLimit
specific permissions - create an Org-specific one, which can be set to Admins, Support, or whomever can set these limits.
|
||
<table class="table table-sm table-bordered" style="margin-bottom: 0;"> | ||
<tr> | ||
<td style="width: 40%;">• System default:</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unicode bullets? This is the first time we're using them, why?
project.upload_limit = 50 * views.ONE_MIB | ||
project.total_size_limit = 50 * views.ONE_GIB | ||
db_request.matchdict["project_name"] = str(project.normalized_name) | ||
result = views.project_detail(project, db_request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add space before action
result = views.project_detail(project, db_request) | |
result = views.project_detail(project, db_request) |
from ....common.db.organizations import ( | ||
OrganizationFactory, | ||
OrganizationProjectFactory, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: move imports to top of module
Organization limits work alongside project and system limits, with the most generous limit always being applied during file uploads. The project admin interface now clearly shows which limit is in effect and where to modify each limit type.
🤖 Generated with Claude Code