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

CoursesUsers should store a copy of user full name, and use that to show instructors #1041

Closed
ragesoss opened this issue Nov 27, 2016 · 8 comments

Comments

@ragesoss
Copy link
Member

commented Nov 27, 2016

This would let us prevent instructors from seeing real names if the user themself didn't join that specific course (but were instead added by the instructor), and also let instructors do batch additions of usernames along with real names (to facilitate grading) without setting the real name on the user record.

@mattwarren10

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

I can take this one, unless someone else is already is doing it.

@ragesoss

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2017

@mattwarren10 sure! Please ask if you have any questions.

Some notes:

  • currently, we store the user's name on User at the time a user goes through the onboarding flow (which is enabled in some cases, and disabled in others). We'll need to copy those names to the existing CoursesUsers objects, and then do the same thing in the future only when a user self-enrolls (via SelfEnrollmentController). Onboarding should still store the name on User.
  • Names get sent to the frontend via a json users endpoint; that will need to be adjusted to get the name from the CoursesUsers rather than User.
  • MassEnrollmentController is the way instructors can add batches of users; that's what will need to be changed to (optionally) handle real names along with usernames. Names added this way would only be stored on CoursesUsers but not on User.

(This task is at the complex end of newcomer-friendly. If you'd prefer something with fewer moving parts, I can make some suggestions.)

@mattwarren10

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2017

@ragesoss,

  • So for the first bullet point, would it be as simple as generating a migration on the courses_users table and then create an instance method through CoursesUsers model that gets the User's real_name? Also I'm not sure what onboarding means.

  • I'm fresh off a new React course, so I'm still fairly a beginner at it. If I look into it enough, I might be able to handle this one. If not I'll let you know.

  • So the MassEnrollmentController would need to call the CoursesUsers model and get either the real_name or the username of the User depending on if they have self-enrolled. Then probably have a form that an instructor can handle more than one user. So when an instructor adds batches of users, names would only be stored on CoursesUsers if self-enrolled. Otherwise, they would be on User. Am I correct?

Also, because of the wikipedia sign-in, how would I create seeds to add students or allow them to self-enroll so that I can see if it works? This is TDD, so create the tests first, correct?

@ragesoss

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

  • That sounds basically right: add a real name column to CoursesUsers, and write a simple script to go through all courses_users and do courses_user.update(:real_name, courses_user.user.real_name) (to be run once manually, after the migration). For onboarding, visit /onboarding while logged in; on one of the production instances, users are redirected here after logging in, if they haven't been through it already. I don't think any changes will be required.
  • Probably no React frontend changes will be required, just a change to app/views/courses/_users.json.jbuilder to get the real name from the CoursesUsers record directly, instead of via the associated User record.
  • There are three places at the controller level where users can be added to a course: UsersController#enroll (for an instructor or admin to add a single student), SelfEnrollmentController#enroll_self (for a user joining a course themself), and MassEnrollmentController#add_users (for an instructor or admin to add a list of users all at once). All three of these ultimately create a new CoursesUsers record via a call to JoinCourse. So the simplest solution here is probably to add an optional real_name parameter to JoinCourse.new, and then pass that real name from SelfEnrollmentController and (if available) from MassEnrollmentController.

You can create an extra Wikipedia account or two for testing purposes.

This should be pretty amenable to TDD. I would start with the migration, and then:

  • Extend JoinCourse to take an optional real_name to save on the new CoursesUsers object.
  • Adjust SelfEnrollmentController to pass user.real_name into JoinCourse.
  • Change the jbuilder and fix any feature specs that break

Extending MassEnrollmentController and allowing it to optionally take real names should be a separate commit (and should have been a separate issue).

@mattwarren10

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2017

So here are my changes. I thought it might be best to let you view them before I make a pull-request.

# db/migrate/20170824162403_add_real_name_to_courses_users.rb
class AddRealNameToCoursesUsers < ActiveRecord::Migration[5.1]
  def change
    add_column :courses_users, :real_name, :string
  end
end
# app/models/courses_users.rb
class CoursesUsers < ActiveRecord::Base
  ...
  def update_real_name    
    CoursesUsers.all.each do |course_user|
      course_user.update(real_name: course_user.user.real_name)
    end
  end
  ...
end
# spec/factories/courses_users.rb
FactoryGirl.define do
  factory :courses_user, class: 'CoursesUsers' do
    nil
    real_name 'John Doe'
  end
end
# spec/models/courses_users_spec.rb
describe '.update_all_caches' do
    it 'updates data for course-user relationships' do
      ...
      create(:courses_user,
             id: 1,
             course_id: 1,
             user_id: 1,
             assigned_article_title: 'Selfie',
             real_name: 'John Smith')
      ...
      expect(course_user.real_name).to eq('John Smith')
    end
end
# app/controllers/self_enrollment_controller.rb
def add_student_to_course
    @result = JoinCourse.new(course: @course,
                             user: current_user,
                             role: CoursesUsers::Roles::STUDENT_ROLE).result,
                             real_name: current_user.real_name)
end
# app/controllers/users_controller.rb
def add
  ...
  @result = JoinCourse.new(course: @course, user: @user, role: enroll_params[:role], real_name: @user.real_name).result
  ...
end

def remove
  ...
  @course_user = CoursesUsers.find_by(user_id: @user.id,
                                        course_id: @course.id,
                                        role: enroll_params[:role],
                                        real_name: @user.real_name)
  ...
end
# app/services/join_course.rb
def initialize(course:, user:, role:, real_name:)
    @course = course
    @user = user
    @role = role
    @real_name = real_name
    process_join_request
end

def create_courses_user
    CoursesUsers.create(
      user_id: @user.id,
      course_id: @course.id,
      role: @role,
      real_name: @real_name
    )
end
# app/controllers/mass_enrollment_controller.rb
def add_user(username)
    user = find_or_import_user(username)
    return { failure: 'Not an existing user.' } unless user
    JoinCourse.new(course: @course, user: user, role: CoursesUsers::Roles::STUDENT_ROLE, real_name: user.real_name).result
  end
# app/views/courses/_users.json.jbuilder
json.call(cu, :character_sum_ms, :character_sum_us, :character_sum_draft, :role, :real_name,
            :recent_revisions, :content_expert, :program_manager, :contribution_url, :sandbox_url)

I haven't extended MassEnrollmentController, but I did add the parameter real_name for calling JoinCourse as displayed above. Let me know what other changes I need to make, or if I left something out. When submitting a pull request would you rather have me merge the new branch into master or submit the pull request without merging?

@ragesoss

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2017

@mattwarren10 This looks like it's going in the right direction, but I have a bunch of inline comments to make. You can open a PR without merging, and I'll add my notes soon after.

@ragesoss

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2017

Left all my comments on the commit.

ragesoss added a commit that referenced this issue Aug 29, 2017
…ttwarren10/WikiEduDashboard into mattwarren10-courses-users-real-name-#1041
ragesoss added a commit that referenced this issue Aug 30, 2017
…urses-users-real-name-#1041

Mattwarren10 courses users real name #1041
@mattwarren10

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2017

This is probably good to close, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.