Override upload file max size in mb

[MuPp3t33r]

For attention to @bradenmacdonald to review if this PR meets requirements.

Description

This update makes the following changes:
constants.js:

• Fix incorrect comment for 20MB default file upload limit
• Retain the default 20MB limit but use an override (if exists) from GetConfig, allowing users to specify an override value via MFE_CONFIG API
• Validation that the value entered is a valid and positive integer (use default in case something goes horribly wrong)
• Written as a function allowing it to be called by other components

FilePage.jsx:

• Remove the hardcoded maxFileSize value and instead import it from constants.js

ModalDropzone.jsx:

• Removed UPLOAD_FILE_MAX_SIZE constants that were defined and replaced with a call to the new function in constants.js

TextbookForm.jsx:

• Removed UPLOAD_FILE_MAX_SIZE constants that were defined and replaced with a call to the new function in constants.js

Imports from constants now use the updated new format '@src/constants' instead of the old '../../constants'

Supporting information

this is an improvement to my previous attempt before i really knew what i was doing, now I only know a little bit of what I'm doing :)

Testing instructions

1. Replace this MFE using a plugin:

from tutormfe.hooks import MFE_APPS

@MFE_APPS.add()
def _override_authoring_mfe(mfes):
    mfes["authoring"] = {
        "repository": "https://github.com/MuPp3t33r/frontend-app-authoring.git",
        "version": "OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB", 
        "port": 2001, 
    }
    return mfes

2. Rebuild your MFE

• tutor images build mfe

3. Apply the patched values to override the config.
   Note that this must be done in MFE_CONFIG and in the CaddyFile to allow Caddy to process your larger requests
   Here is a sample plugin allowing user to provide one input for the value and it applies in both locations

from tutor import hooks

# Instructions / info
# override_value is defined as a string so bad formats (incorrectly entered values) don't crash Python immediately
# User MUST enter only digits as a POSITIVE integer representing a value in MegaBytes (MB), e.g. "1024" for 1GB
# This adds the value to the MFE_Config API as well as the CaddyFile CMS block

override_value = "1024"  

# --- Validation ---
try:
    override_int = int(override_value)
    if override_int <= 0:
        raise ValueError
except ValueError:
    raise ValueError(
        f"Invalid override_value: {override_value}. "
        "It must be a positive integer without units (e.g., 1048)."
    )

# --- Config patches ---
hooks.Filters.ENV_PATCHES.add_items([
    (
        "openedx-lms-production-settings",
        f"""
MFE_CONFIG["OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB"] = "{override_int}"
"""
    ),
])

hooks.Filters.ENV_PATCHES.add_item(
    (
        "caddyfile-cms",
        f"""
# Maximum asset upload size in CMS/Studio
handle /assets/* {{
    request_body {{
        max_size {override_int}MB
    }}
}}
        """
    )
)

4. Finally - enable the override plugin and restart your instance:

• tutor plugins enable override_max_asset_upload_size
• tutor local stop && tutor local start -d

Now, under the following menus:

• Content -> Files
• Content -> Pages & Resources -> Textbooks
  You will find that you can upload files up to whatever maximum defined in the override_max_asset_upload_size plugin

[openedx-webhooks]

Thanks for the pull request, @MuPp3t33r!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[e0d]

@MuPp3t33r Thanks for your detailed PR description. I've approved the tests to run here and will await @bradenmacdonald 's review.

[e0d]

@MuPp3t33r I notice there are some commit-lint failures. Please note that we use conventional commits across Open edX projects. You can read about the details here. Can you please amend your commit messages to follow our standard?

[bradenmacdonald]

@MuPp3t33r Thanks for the revised version here, and especially for the nice testing instructions! I will try to review this shortly.

In the meantime, would you mind addressing these minor lint issues? And as @e0d mentioned, we need the commits to start with feat: in order to pass the check. Let me know if you want help with how to do that.

[MuPp3t33r]

@e0d @bradenmacdonald
Hopefully I've done it right this time and not butchered anything else in the process :)
Would you mind re-running the tests to confirm?

[e0d]

I've run them.

[MuPp3t33r]

@e0d I'm so sorry to keep wasting your time, my experience (or lack thereof) with github clearly shows. with any luck i've done it right this time... Sincerely appreciate your patience with me :)

[bradenmacdonald]

This looks great! Just need one fix to get the tests passing again:

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.55%. Comparing base (7825bcd) to head (574a34c).
⚠️ Report is 116 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2370      +/-   ##
==========================================
+ Coverage   94.50%   94.55%   +0.05%     
==========================================
  Files        1171     1174       +3     
  Lines       25149    25372     +223     
  Branches     5374     5557     +183     
==========================================
+ Hits        23766    23991     +225     
+ Misses       1320     1312       -8     
- Partials       63       69       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bradenmacdonald]

@MuPp3t33r This is almost ready to merge! We just need to add a test to get the coverage check passing.

Would you mind adding something like this? I think that would do it.

// src/constants.test.ts
import { mergeConfig } from '@edx/frontend-platform';
import { getUploadFileMaxSize } from './constants';

describe('getUploadFileMaxSize()', () => {
  it('returns the global default when no config value is set', () => {
    expect(getUploadFileMaxSize()).toEqual(20 * 1024 * 1024);
  });

  it('returns OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB from the config when set', () => {
    mergeConfig({ OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB: 7 })
    expect(getUploadFileMaxSize()).toEqual(7 * 1024 * 1024);
  });
});

[MuPp3t33r]

Thanks @bradenmacdonald!
For the sake of completeness, should we also have tests for other input conditions? Something like this perhaps?
If you disagree I'll try with just your code...

// src/constants.test.ts
import { mergeConfig } from '@edx/frontend-platform';
import { getUploadFileMaxSize } from '@src/constants';

const DEFAULT_MAX = 20 * 1024 * 1024;

describe('getUploadFileMaxSize()', () => {
  afterEach(() => {
    // Reset config after each test to avoid leaks
    mergeConfig({});
  });

  it('returns the global default when no config value is set', () => {
    expect(getUploadFileMaxSize()).toEqual(DEFAULT_MAX);
  });

  it('returns OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB when set to a valid positive integer', () => {
    mergeConfig({ OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB: 7 });
    expect(getUploadFileMaxSize()).toEqual(7 * 1024 * 1024);
  });

  it('falls back to default when override is not a number', () => {
    mergeConfig({ OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB: 'not-a-number' as any });
    expect(getUploadFileMaxSize()).toEqual(DEFAULT_MAX);
  });

  it('falls back to default when override is 0', () => {
    mergeConfig({ OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB: 0 });
    expect(getUploadFileMaxSize()).toEqual(DEFAULT_MAX);
  });

  it('falls back to default when override is negative', () => {
    mergeConfig({ OVERRIDE_UPLOAD_FILE_MAX_SIZE_IN_MB: -5 });
    expect(getUploadFileMaxSize()).toEqual(DEFAULT_MAX);
  });
});

[MuPp3t33r]

I just realised I should probably also update the README.rst to document the new optional configuration, is this the ideal place to document it or is there also another documentation that this would be relevant for?

[bradenmacdonald]

@MuPp3t33r Yes, that would be even better - thanks!

As for docs I'm not aware of the MFE config being properly documented anywhere :/ I think adding it to the README makes sense for now.

[MuPp3t33r]

Thanks @bradenmacdonald
tests added, and I've updated the readme and created a supplementary how-to file with usage examples. hopefully this should cover all bases.

[MuPp3t33r]

its been brought to my attention that there is actually still an issue with this, so it's not yet ready for merge!
turns out that MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB is actually still used in the handling of file uploads via the MFE so the default there of 100MB causes larger files to fail even though the MFE does display the new limits correctly. I'll need to have a look into how I can get these settings to talk to each other better...

[MuPp3t33r]

Hi @bradenmacdonald hoping to just get your thoughts\opinion on this:

I see in views.py#L97 there is code to get legacy configs which I'm guessing we could use to import the legacy config MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB and use it's value for our override.

But based on what I've seen around other places (and in that same file,) these "will eventually be deprecated"

Does this mean that eventually all legacy configs will be deprecated in favour of the MFE configs? for example like the asset_storage_handlers.py code here which if I'm not mistaken is still used in conjunction with the MFE's as well?

To clarify: even with the authoring mfe's default const UPLOAD_FILE_MAX_SIZE = 20 * 1024 * 1024; the MFE will show max allowed size = 20MB, however, if the legacy config MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB were to be set to 10MB, then the MFE will still display 20MB limit but will fail on anything over 10MB.

I mean I could simply update my patch plugin to modify all the relevant values, but having to patch so many things in so many places doesn't sit right with me for what I'd hope to be a simple config, especially if old stuff will be deprecated anyway.

Appreciate your input, still trying to wrap my head around everything to be honest :)

[bradenmacdonald]

@MuPp3t33r Configuration in our MFEs is a bit of a mess right now. You're asking good questions, but I don't actually know the answer. @arbrandes or @brian-smith-tcril would one of you mind clarifying these questions about which configuration systems will be used in the future, and where a config value for maximum upload sizes in Studio / Authoring MFE should live?

I would assume that we should either (A) specify the config only in edx-platform (where it would be enforced by the API) and pull platform's value into the MFE using (some mechanism - runtime config? studio home data?); or (B) have very simple, separate settings in edx-platform and Authoring MFE and make sure the defaults are in sync.

[mphilbrick211]

@MuPp3t33r Configuration in our MFEs is a bit of a mess right now. You're asking good questions, but I don't actually know the answer. @arbrandes or @brian-smith-tcril would one of you mind clarifying these questions about which configuration systems will be used in the future, and where a config value for maximum upload sizes in Studio / Authoring MFE should live?

I would assume that we should either (A) specify the config only in edx-platform (where it would be enforced by the API) and pull platform's value into the MFE using (some mechanism - runtime config? studio home data?); or (B) have very simple, separate settings in edx-platform and Authoring MFE and make sure the defaults are in sync.

Friendly ping on this @brian-smith-tcril @arbrandes

[brian-smith-tcril]

@MuPp3t33r Configuration in our MFEs is a bit of a mess right now. You're asking good questions, but I don't actually know the answer. @arbrandes or @brian-smith-tcril would one of you mind clarifying these questions about which configuration systems will be used in the future, and where a config value for maximum upload sizes in Studio / Authoring MFE should live?

I would assume that we should either (A) specify the config only in edx-platform (where it would be enforced by the API) and pull platform's value into the MFE using (some mechanism - runtime config? studio home data?); or (B) have very simple, separate settings in edx-platform and Authoring MFE and make sure the defaults are in sync.

For the Catalog MFE we are DEPRing some legacy config, and for the expand/contract process we're exposing the legacy values as fallbacks for new MFE_CONFIG entries (that can be consumed by MFEs using getConfig() at runtime). See openedx/openedx-platform#37130.

If there is a file size limit set on the backend, then we should be reading that in the MFE. I think possibly following a similar pattern (expose the existing value via MFE config, not as a fallback, but as the source of truth for that config value) then the MFE could:

• Still read from getConfig and have tests work
• Always get the correct value from the backend

[MuPp3t33r]

Thanks for the feedback @brian-smith-tcril
If I'm understanding correctly, the appropriate solution here is to update the _get_legacy_config definition to expose the value of MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB to the MFE config API. This way there's no disconnect between old and new settings, making the legacy config the official source of truth to the MFE

If you can confirm I'm not misinterpreting anything then I'll close this off and try work up something more appropriate before opening a new PR... Thanks :)

[brian-smith-tcril]

Thanks for the feedback @brian-smith-tcril If I'm understanding correctly, the appropriate solution here is to update the _get_legacy_config definition to expose the value of MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB to the MFE config API. This way there's no disconnect between old and new settings, making the legacy config the official source of truth to the MFE

That sounds good to me, but I'd want to check with someone with edx-platform expertise like @feanil to make sure that plan/pattern seems reasonable as it would mean _get_legacy_config isn't something that would be removed after DEPRs, but instead something that would be used more broadly.

[MuPp3t33r]

I see :) thanks. I'll wait for a platform expert like @feanil to weigh in on this before proceeding with anything...

[mphilbrick211]

I see :) thanks. I'll wait for a platform expert like @feanil to weigh in on this before proceeding with anything...

@feanil are you able to help out here?

[feanil]

I added a couple of other notes while I was looking at this.

The existing _get_legacy_config code is meant to be temporary to expose settings that are specific to MFE behavior that were previously being referenced by legacy templates served from the backend, but aren't something we plan on keeping in the backend forever. I don't think that's the right spot to expose this setting from the backend. Though I do agree that this setting should be exposed and the frontend and backend should be aligned on the correct shared value of this backend limit.

Unfortunately there is not currently a backend API for serving metadata related to asset management that is relevant to expose to the frontend yet, based on looking at the currently defined views: https://github.com/openedx/edx-platform/blob/master/cms/djangoapps/contentstore/views/assets.py#L31 and under https://github.com/openedx/edx-platform/tree/master/cms/djangoapps/contentstore/rest_api

I think the correct thing to do is to build a new DRF endpoint that exposes the max filesize data to the frontend. It feels a bit silly since this page doesn't need any other info that we could bundle together into a single call so the call seems kind of large for the small amount of data we're fetching but I think it's architecturally correct and if we do have more metadata we want to provide to this page in the future, we'd expand this call to expose that metadata.

So I think you want an edx-platform PR that adds a new endpoint to contentstore api, probably here: https://github.com/openedx/edx-platform/blob/master/cms/djangoapps/contentstore/rest_api/v2/urls.py

Then in this PR you'd use that endpoint to fetch the data to set the frontend limits.

[feanil]

We don't use the overheading in this project.

Docs if you want to know more about our style guide: https://docs.openedx.org/en/latest/documentors/references/quick_reference.html#documentation-syntax-reference

[feanil]

Should this function should live in utils.tsx and not in constants.js?

[feanil]

Putting OVERRIDE in the name is redundant for a config setting. I think MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB is a clear name. Is there a reason not to use the same name we use in the backend as the name of the setting in the mfe config and as the name of the constant in the frontend? Unless there is a change in the meaning of the constant, keeping the name the same should reduce cognitive load for future readers of the code.

[MuPp3t33r]

Thank you @feanil, with that I think I've got enough information to give this another try in my spare time. For now I'm going to close this PR since my implementation was so far off the mark...
A huge thanks to everyone in this conversation for your input :)