Refactor researcher classes API

[pjanik]

Related to the Slack discussion:
https://concord-consortium.slack.com/archives/C0M5CM1RA/p1711115781067969

There will be more changes in this controller, but I just wanted to double-check if the concept is correct.

classes_query is the only query that explicitly applies filters. That's the "output".
Remaining queries: teacher_query, cohorts_query and runnables_query accept classes_ids_subquery and don't apply filter values explicitly. @scytacki, I understand that this is what you meant by subqueries, right?

This means that we'll usually execute 4 DB queries.

I could see another option. We could modify the final processing of classes to something like this:

  def classes_mapping(classes_query)
    teachers = []
    cohorts = []   
    runnables = []

    classes = classes_query.map do |c|
      cohorts << c.cohorts
      teachers << c.teachers
      runanbles << c.runnables
      
      hash = {}
      hash[:name] = c.name
      # ...
    end
  
    # return all 4 arrays

(ofc we probably shouldn't use arrays to deal with duplicates, etc., but it's just a general idea)

It would be just one explicit query execution (for classes), but I guess collecting teachers, runnables, and cohorts from each class could actually trigger lots of additional queries for each class, right? Or do I miss a third way to approach it?

[scytacki]

Yeah the class_mapping function is the simple way to do it. But it will likely have memory and performance problems as all of that data has to be turned into ruby objects and then additional queries executed to get the other field values.

The other part is that we might eventually want something like the first X field values to show in the field drop down unless the user starts searching for more or starts to scroll past the first X. So then to get those first X, if the field value has less than X unique values, you'd have to loading in every single class.

Perhaps there are few enough classes that this is OK, but I'd guess there are too many.

I'm going to look at the code now...

[scytacki]

This looks great to me.
As I hoped, it seems to remove a lot of code.

It might be possible to convert some of the SQL to active record query format without losing any performance, but I don't know if that is worth it. If our ruby code was more typed like typescript, that could make refactoring safer. But without typing most of our refactoring is based on string searches, so I'm not sure how much using more active record query really gains in something like this.