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.

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

@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?

@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.

@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?

I've run them.

@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 :)

Codecov Report

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

@@            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.

@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);
  });
});

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);
  });
});

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?

@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.

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.

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...

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 :)

@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.