Allow courses to set Matlab API key globally, in advanced settings.

[davestgermain]

@andy-armstrong @rocha

[andy-armstrong]

This looks good, but I thought that you were going to put the new setting on the course. I'm not familiar enough with how these settings work so maybe this has the same effect. Is the user able to specify the settings once per course? Also, why does the setting have to be added to both capa_problem and capa_base?

I'll check this out in detail tomorrow, but I figured I'd throw out my initial questions up front.

[rocha]

Is is possible to make this more generic. I don't like having a parameter that as explicit at matlab_api_key at such a high level in the class hierarchy of CAPA. I'll be ok if the parameter only affects matlab related components.

One alternative is using something like:

advanced_options = Dict(help="advance configuration options for capa components", ....)

An then, on the component, query for the matlab_api_key specifically:

api_key = getattr(self.capa_system, 'advanced_options', {}).get('matlab_api_key');

I am unsure how this will look from Studio however.

[davestgermain]

I don't particularly like it either, but on the other hand, calling it advanced_options obscures the purpose, especially since there's only one option right now.

I was surprised there wasn't already a generic way of getting key-value configuration from the course into individual input types.

[rocha]

What about something like course_advanced_settings? I really don't like having something so specific at the CapaFields level.

[davestgermain]

But then the user experience would be that they go to the course advanced settings page and see a bunch of fields, one of which is called course_advanced_settings, and then they have to know that it's a dictionary and need to enter matlab_api_key as one of the keys...

What do you think, @cpennington?

[rocha]

@davestgermain I am not addressing the user experience, just how the code looks in the changes you are making.

[cpennington]

Another option, potentially, would be to read additional fields from the set of installed input types. Then the matlab_api_key would be defined by the input type, and just added to capa_base because that's the actual xblock involved.

I don't think a generic course_advanced_settings is the way to go. I'd rather have a specific field for this configuration than to add a generic bucket like that.

[davestgermain]

user experience is the most important thing. the settings page is already confusing as it is. let's get another opinion.

[rocha]

@davestgermain I agree with user experience is the most important thing. What I meant is that so far my comments have not addressed that. I have only considered how the changes in the code is structured.

I like @cpennington idea.

[davestgermain]

@cpennington @rocha it's not clear to me how to add settings to inputtypes in such a way that they'll propagate up, and it sounds like a major change for little benefit. no?

[cpennington]

In this case, yes, I think it's overkill. If we find more of this sort of thing before we manage to make InputTypes and ResponseTypes into xblocks, then we can revisit it then.

[davestgermain]

@andy-armstrong I added the API key to the CAPA xblock because @cpennington suggested that'd be the appropriate place for it. It has to be added in several places because of the peculiarities of how settings are propagated through the system, but the end result is that a matlab_api_key field shows up in the advanced settings of studio.

[rocha]

@davestgermain I suggest you also get someone from the LMS team to review this PR. @dianakhuang can help you to find someone.

[rocha]

The two blocks above are a bit confusing. The above is a bit confusing. Can you add some comments as for why we are parsing the values in this way? Specially why we are making a plot_playload with the api_key.

[davestgermain]

@rocha I added a couple of comments.

[andy-armstrong]

I don't think I can add anything here, so I second @rocha's suggestion of getting an LMS reviewer instead. It seems that if this is configured correctly for LMS then the Studio pieces just work.

[rocha]

If the folks from the LMS are OK with the change I am good too. I don't have the expertise here.

[cpennington]

👍

[carsongee]

I just caught wind of this from MatLab. Is there a reason we maybe don't make this more generic so other external graders could potentially use it? Perhaps just grader_api_key or similar with approprriate help changes?

[cpennington]

Because we're expecting to inherit it from the course level, calling it grader_api_key won't work (because a course might have multiple grader types, and those might each have their own keys).

One thing that I've been thinking about is making it so that fields can be marked as inheritable on the blocks that they are defined on, and the LMS/Studio can simply collect all of those fields. That way, adding new fields as inheritable won't be quite as cumbersome.

[carsongee]

I see. That makes sense. I definitely like the idea of controlling inheritance at the block level would be nice to avoid changing inheritance.py all the time. I haven't looked, but do XBlocks allow you to specify fields that should be presented at the course level? If not, then definitely 👍 to making that definable at the module/block level

[cpennington]

Currently, no. My thought has been to allow Field definitions to take arbitrary kwargs that would be interpretable by specific runtimes (perhaps with an eventual standardization process).

[sarina]

This PR needs to be rebased to make sure it works well with opaque-keys. Is there anything else blocking it from being merged?

[davestgermain]

@sarina I'm waiting to hear back from Mathworks to verify that their end of the API matches this side.

[sarina]

@davestgermain ok that makes sense. How long do you think that will take? (I'm in spring cleaning mood and want to get all these old PRs closed!)

[davestgermain]

Mathworks has been slow to respond, but at the same time they said they want to deploy their end next week. So, I'll merge this now, and if their API isn't quite what's here, it can be tweaked next week.