fix: change teacher class API to accept Class ID instead of TeacherClass ID #1244
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes two bugs present on the Manage Classes page:
[Portal] Unable to copy a class
[Portal] Unable to sort classes
In short, the issue was that the API was designed to deal with Portal::TeacherClazz model IDs, while the client was sending Portal::Clazz IDs. It seems it should break easily, but often these IDs match - usually as long as some model instances are not deleted, or until multiple teachers are added to one class. That's why I could not reproduce this issue locally, while it was failing on staging and production.
There are two obvious solutions to this problem:
A. Change the client to use the TeacherClazz model and post these IDs to the API.
B. Change the API to use Clazz model IDs instead of TeacherClazz IDs.
I opted for option B. I think that using
TeacherClazzdoesn't bring any benefits, in my opinion, and it makes the API, its implementation, and the client a bit more confusing. When you copy a class, you copy a class, not a link between a teacher and a class. I could imagine that being useful only if an admin would like to copy a class on behalf of a teacher, and while copying a class, the new one should be assigned only to this specific teacher (not all owners of the previous class). But this scenario wouldn't really work with the current API, even if it were supported by the rest of the Portal.The sorting of classes is slightly different, as in this case, we actually sort the link between the class and the teacher. But still, for client simplicity and consistency with the rest of the API, I think using the class ID is justified.