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

Fixed not populating invited_by field for coteacher invitations #54467

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

lfryemason
Copy link
Contributor

The invited_by field in SectionInstructor was not being populated correctly because the add_instructor method in sections.rb was not able to correctly identify the current_user here. FLUP to this PR

This PR fixes the bug and adds a test that invited_by is correctly filled out.

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

@etaderhold etaderhold left a comment

Choose a reason for hiding this comment

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

Huh, current_user doesn't work from model classes? TIL.

@@ -584,7 +584,7 @@ def update_code_review_expiration(enable_code_review)
end
before_validation :strip_emoji_from_name

public def add_instructor(email)
public def add_instructor(email, current_user)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think invited_by is a better descriptor of what this parameter is actually for.

Suggested change
public def add_instructor(email, current_user)
public def add_instructor(email, invited_by)

Switching it up to keyword parameters instead of positional parameters would make the purpose of passing current_user more clear in the calling sites, at the cost of slightly longer code. I'll leave it up to you whether that's worth it.

Suggested change
public def add_instructor(email, current_user)
public def add_instructor(email:, invited_by:)

@lfryemason lfryemason merged commit 6eceb22 into staging Oct 25, 2023
2 checks passed
@lfryemason lfryemason deleted the lfm/co-server-fix branch October 25, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants