zeeguu / api

API for tracking a learner's progress when reading materials in a foreign language and recommending further personalized exercises and readings.
https://zeeguu.org
MIT License
8 stars 23 forks source link

198 students invisible when joining second classroom #206

Closed tfnribeiro closed 1 month ago

tfnribeiro commented 1 month ago

Change to support the UI changes to allow multiple classrooms for one user (https://github.com/zeeguu/web/pull/526)

Changelist

Question:

(Before commiting @mircealungu)

github-actions[bot] commented 1 month ago

Module view diffs: diff

mircealungu commented 1 month ago

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.

tfnribeiro commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.