Separation of validation versus load logic discussion

[hepcat72]

Thanks for the thoughtful reply @hepcat72. I think we are on the same page as far as design goes. It might be useful to continue this conversation in a different context, since I think this PR can and should get merged soon. My initial thoughts are that the management commands should contain the CLI/curator type logic, the validation controllers should contain the logic for the user validation, and both should call the same underlying load utilities methods. I believe that separation could be achieved, but I'm not sure it's worth the extra effort at this point, perhaps if we go through this code again in the future.

Originally posted by @lparsons in #618 (comment)

both should call the same underlying load utilities methods

Yes. If the validation view and load_study both contain the same code (with the validation view adding some data structures to track outcomes in a granular fashion), the output of that process can be handled differently. That's where the original code was heading, but the thing that really bothered me about that was the code duplication and the maintenance overhead that would come with it. I think that eliminating the logic redundancy outweighs the logic separation. That means that when someone edits the load_study code, they don't have to also edit the validation code. And I think they shouldn't have to.

But maybe there's another way to achieve the code re-use and eliminate the overhead that would come from the redundancy. All we need to solve is how to get the status output of the loading logic to the caller and the caller can decide what to do with it. The caller can either be load_study or the validation view. load_study and the validation view can each call this underlying code and do whatever it decides with the return. E.g. load_study can decide to print warnings to the console whereas the validation view can package it for presentation in the template. All the underlying code has to do is return the status of each load script. It could take a parameter to indicate whether the results should be committed or not.

To stub this out, let's create methods named:

• process_study(commit=False) - does everything the current load_study.py does, but only commits if commit is True and returns a status data structure
• validate_study() - calls process_study(commit=False)
• load_study() - calls process_study(commit=True)

process_study() would create an exception for everything (warnings and errors). And load_study() and validate_study() would decide whether any of them should be warnings, errors, or whatever and decide whether they should be printed or packaged up to send to a template.

I think that's the best of both worlds. All the loading logic exists in 1 place and the logic of what to do in each case is separate based on the context.