198 students invisible when joining second classroom

[tfnribeiro]

Change to support the UI changes to allow multiple classrooms for one user (zeeguu/web#526)

Changelist

• Removed the cohort_id from the user table, instead a mapping table was introduced to track this relationship.
• The script first inserts all the cohort_id into the mapping table, so users remain in their current classrooms.
• Testing membership of users is now done by a method is_member_of_cohort which expects an ID and checks that mapping exists for the user.
• basic_user_info now sends a list of cohorts if the user is in multiple classrooms.
• Change methods to call their new equivalent, overall maintaining the same logic the end-points used to have. This mostly means that now we need to check for multiple entries and find if a specific ID is present.
• TeacherDashboard now reflects the time spent on exercises for the specific language, rather than all exercises done by a student.

Question:

(Before commiting @mircealungu)

• When we add/remove a user from a cohort, should we commit immediately or add it to the session and then the developer can decide when to commit the change?

[github-actions]

Module view diffs:

[mircealungu]

When we add/remove a user from a cohort, should we commit immediately or add it to the session and then the developer can decide when to commit the change?

are you talking about this?
https://github.com/zeeguu/api/pull/206/files#diff-bb192734c50e63f5c18ff35dcab929883ceadefef56fa5c32fef107d21bf7d2eR321

It s ok how you're doing it. Although every commit takes a bit of time.

We could try to create a rule of thumb that says that something like:

things should be committed by the caller if possible

or

if a method gets a session, it should modify it, but not commit by default

So in this case then the add_user_to_cohort would just add the user to the session but leave the commit to be done by the parent.

However, I'm not sure that there won't be exceptions from these rules.

[mircealungu]

that was quite a bit of work.
looks good overall.
minor asks and we're ready to deploy.

[mircealungu]

this comment is off?

[tfnribeiro]

I didn't edit it, seems like it has been this way for 6 years - but I can remove it - I think it came in the diff due to the formatting.

[mircealungu]

really? then maybe we should remove it?

[tfnribeiro]

Seems that way, I believe it was something that we didn't end up using when Iga did the onboarding.

[mircealungu]

Whoa! you can do that w/o a session :)

[tfnribeiro]

When we add/remove a user from a cohort, should we commit immediately or add it to the session and then the developer can decide when to commit the change?

are you talking about this? https://github.com/zeeguu/api/pull/206/files#diff-bb192734c50e63f5c18ff35dcab929883ceadefef56fa5c32fef107d21bf7d2eR321

It s ok how you're doing it. Although every commit takes a bit of time.

We could try to create a rule of thumb that says that something like:

things should be committed by the caller if possible

or

if a method gets a session, it should modify it, but not commit by default

So in this case then the add_user_to_cohort would just add the user to the session but leave the commit to be done by the parent.

However, I'm not sure that there won't be exceptions from these rules.

Yes, it's just that in the code you can see in the addition I do the commit in the method in add user, while in the delete I am doing it in the API call rather than the User.Method, where it just adds the value to be deleted to the session, it can then be deleted at the API level. I think that it creates a bit of a conflict in the semantics of add/delete user, if the user is not really added or deleted. What do you think we commit, and if we decide to use sessions, maybe we should call the method "mark_for_delete/add" - so it's more explicit it doesn't delete the user right away.

[mircealungu]

When we add/remove a user from a cohort, should we commit immediately or add it to the session and then the developer can decide when to commit the change?

are you talking about this? https://github.com/zeeguu/api/pull/206/files#diff-bb192734c50e63f5c18ff35dcab929883ceadefef56fa5c32fef107d21bf7d2eR321
It s ok how you're doing it. Although every commit takes a bit of time.
We could try to create a rule of thumb that says that something like:

things should be committed by the caller if possible

or

if a method gets a session, it should modify it, but not commit by default

So in this case then the add_user_to_cohort would just add the user to the session but leave the commit to be done by the parent.
However, I'm not sure that there won't be exceptions from these rules.

Yes, it's just that in the code you can see in the addition I do the commit in the method in add user, while in the delete I am doing it in the API call rather than the User.Method, where it just adds the value to be deleted to the session, it can then be deleted at the API level. I think that it creates a bit of a conflict in the semantics of add/delete user, if the user is not really added or deleted. What do you think we commit, and if we decide to use sessions, maybe we should call the method "mark_for_delete/add" - so it's more explicit it doesn't delete the user right away.

this is an interesting idea, but i wonder if it won't make our method names too long?

    def mark_for_add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_cohort = UserCohortMap(user=self, cohort=cohort)
        session.add(new_cohort)

the more we discuss the more I think that somebody who gets a session should not commit it.
you get a session you add to it, and that's it.

    def add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_cohort = UserCohortMap(user=self, cohort=cohort)
        session.add(new_cohort)

alternative would be to not pass the session at all anymore, but instead such methods that aim to modify the DB model always return the objects that have to be added to the session and committed?

e.g.

    def add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_user_cohort_map = UserCohortMap(user=self, cohort=cohort)
        return new_user_cohort_map

then the caller, probably the api endpoint who already has a session handy does

    new_mapping = flask.g.user.add_user_to_cohort(cohort)
    session.add(new_mapping)
    session.commit()

do you see what I mean?

surely, this might become complicated, because when you want to add multiple objects then you have to have to create a temporary array to which you add all these objects, and then you return it. So probably the solution above with passing the session and adding directly to it is still the most concise. And functionally it's exactly the same as the one with returning the objects to be added. However, in a sense it's more clear that things have to be added to the session.

[tfnribeiro]

I do see it - and generally I feel like it makes more sense that things are added rather than commited immediately especially if we were to do bulk updates, then it would be much better to store it in session. I think returning the altered object could be a good solution for this - and we can slowly start implementing it.

Should I create an issue where we can track it?

[mircealungu]

Just to be on the same page, I still think the best approach would be to simply pass the session, and add things to it. To be sure that we're on the same page with this, I've created a Discussion category called Coding Guidelines and also a separate file Coding Conventions.