zulip icon indicating copy to clipboard operation
zulip copied to clipboard

realm: Change implementation approach for upload_quota_gb.

Open mateuszmandera opened this issue 1 year ago • 3 comments

Important: When deploying this to zulipchat.com, it's necessary to immediately set custom_upload_quota_gb for any organizations with a custom upload quota, to avoid having their quota get reset to the default implied by the plan.

Most importantly, fixes a bug where a realm with a custom .upload_quota_gb value (set by changing it in the database via e.g. manage.py shell) would end up having it lowered while upgrading their plan via the do_change_realm_plan_type function, which used to just set it to the value implied by the new plan without caring about whether that isn't lower than the original limit.

The new approach is cleaner since we don't do db queries by upload_quota_gb so it's nicer to just generate these dynamically, making changes to our limit-per-plan rules much easier - skipping the need for migrations.

mateuszmandera avatar Mar 11 '24 01:03 mateuszmandera

Still working on webapp fixes to upload_quota_mib, so that commit not ready to merge yet

mateuszmandera avatar Mar 20 '24 01:03 mateuszmandera

Okay, webapp is fixed now, so should be good to review. I replied to things above and fixed the upload_quota_mib issue

mateuszmandera avatar Mar 20 '24 03:03 mateuszmandera

Sorry for the slow review! I think the only outstanding detail is using caching to save queries? Feel free to DM me for a quick merge once you've got this ready, so we can avoid further renumbering work.

timabbott avatar Apr 05 '24 17:04 timabbott

I resolved the version.py merge conflict and added this double-entry detail for the API changelog:

diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml
index e85d061980..5c5dd02f45 100644
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@@ -15277,7 +15277,10 @@ paths:
 
                           If `null`, there is no limit.
 
-                          **Changes**: New in Zulip 5.0 (feature level 72). Previously,
+                          **Changes**: Before Zulip 9.0 (feature level 251), this field
+                          was incorrectly measured in bytes, not MiB.
+
+                          New in Zulip 5.0 (feature level 72). Previously,
                           this was called `realm_upload_quota`.
                       realm_org_type:
                         type: integer

With those changes, merged, thanks @mateuszmandera!

I think https://github.com/zulip/zulip/pull/29258#discussion_r1566471387 could be worth doing while we're touching this, but it's looking like it's own nontrivial refactor, so probably deserves its own PR so that we can integrate the main API change/migration.

timabbott avatar Apr 15 '24 21:04 timabbott