Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike: Support multiple teachers per class #456

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

loiswells97
Copy link
Contributor

Status

  • Closes add issue numbers or delete
  • Related to add issue numbers or delete

Points for consideration:

  • Security
  • Performance

What's changed?

Description of what's been done - bullets are often best

Steps to perform after deploying to production

If the production environment requires any extra work after this PR has been deployed detail it here. This could be running a Rake task, a migration, or upgrading a Gem. That kind of thing.

@cla-bot cla-bot bot added the cla-signed label Nov 1, 2024
@loiswells97 loiswells97 temporarily deployed to editor-api-p-spike-mult-xe44vc November 1, 2024 16:37 Inactive
@loiswells97 loiswells97 changed the title inconsequential change to generate pr Spike: Support multiple teachers per class Nov 1, 2024
@@ -1,6 +1,6 @@
# frozen_string_literal: true

class ClassMember
module ClassMember
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being curios, why do switch class here to a module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we renamed the class_member table in the database to class_student, and also renamed the ClassMember model/class to ClassStudent. So we don't really care about creating instances of ClassMember anymore, just in using the methods it provides. These methods in some cases were taking account of both teachers and students, even though instances of the old ClassMember model could only represent students, which was confusing, hence the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for open anwer👏

Copy link
Contributor

@KristinaDudnyk KristinaDudnyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants