Updates to adding users to newly created courses. #2619

somiaj · 2024-11-09T15:55:09Z

When adding a course on the course admin page, list all users in the admin course and let the admin select which users to copy over to the new course (by default all admin users are selected). This way an admin can select an existing user to copy over which can include any password or OTP secrets setup for that use (currently the admin will have to copy that over to the admin course manually). This also allows only copying a subset of admins to a newly created course instead of all.

When creating a new user to add to a course, be consistent with adding a user on the accounts management page, which doesn't require a password (useful when using external auth), email, etc. Now the only required field is the new user ID.

Flag the new user password input as 'new-password', so webbrowsers don't try to auto fill it in. Add the student ID as a field they can fill out instead of making it equal to the user ID. Since users can be copied from the admin course, add an option to add the new user to the admin course (as a dropped user so they can't login) so admins can select the same user to add to future courses as needed.

Remove the empty list of sets to assign users to when adding new users on the accounts management page in the admin course.

Make it so all users are shown on the Email page in the admin course, since most instructors are "Dropped" to still allow sending them emails.

Alex-Jordan · 2024-11-09T20:40:04Z

This sounds nice. I have "math" admin users in the admin course, as well as "chemistry" admin users. It will be nice to have a way to easily exclude one set or the other from new courses.

Alex-Jordan · 2024-11-10T23:55:57Z

lib/WeBWorK/ContentGenerator/CourseAdmin.pm

+				$db->addUser($User);
+				$db->addPassword($Password);
+				$db->addPermissionLevel($PermissionLevel);
+				$User->status('O');


Is this line meant to be here?

Yes, the status needs to be set back to observer before that user is copied into the new course (otherwise they would be copied into the new course as dropped).

Though I have been thinking that maybe dropping users by default isn't the best approach here (I just don't want them able to log into the admin course). Instead the admin course could probably be configured that only admins can log into it via a course configuration, but this would have to be done in course.conf.

I think I'll change it to not make users in the admin course dropped by default (maybe an admin wants to allow these users to login to the course so they can set passwords or setup two factor auth, to then be copied to new courses). Though we should probably add something about making a course.conf for the admin course that prevents anyone except admins from logging in as default if they decide to add non admin users to it.

I don't think that is is wise. I think better would be to make the user dropped by default. That can always be changed from the accounts manager if desired.

Alex-Jordan · 2024-11-10T23:57:02Z

In Firefox, I am seeing a scroll border at the bottom even though horizontal scrolling is not in play:

Does changing the overflow-scroll to overflow-scroll-y make sense?

Can we change "Add new user to admin course." to something like "Also add this new user the ___ course." where the blank is the actual name of the admin course?

somiaj · 2024-11-11T00:06:58Z

Strange, in firefox here I only see the vertical scroll bar, do you have a user with a really long name down below causing the scroll bar to appear. I guess I could do overflow-scroll-y, but then if there is a really long user name (role) it won't be possible to see it.

Yea, I'll update it to use the admin course name.

drgrice1 · 2024-11-11T00:33:36Z

templates/ContentGenerator/CourseAdmin/add_course_form.html.ep

-				<%= check_box 'add_admin_users' => 1, class => 'form-check-input' =%>
-				<%= maketext('Add WeBWorK administrators to new course') =%>
-			</label>
+		<div class="col-lg-4 col-md-5 col-sm-6 overflow-scroll border rounded px-2" style="height: 100px;">


I think you should use max-height here instead of height. Then if there are less than 5 users in the admin course (my admin courses never have more than 1) the box isn't so large.

drgrice1 · 2024-11-11T00:34:12Z

Change the overflow-scroll to overflow-auto. Then the scroll bars will only show up if needed.

templates/ContentGenerator/CourseAdmin/add_course_form.html.ep

somiaj · 2024-11-11T00:51:52Z

Updated with the suggested changes. I reverted then enabled the dropping new users by default (they are still dropped by default). Though I did leave the comment about using $permissionLevel{login}='admin' in the course.conf file as a way to control access. That might be useful since there is no "Course Configuration" in the admin course.

drgrice1 · 2024-11-11T01:07:10Z

Do we have to add the student ID to the page? That makes that part of the page look rather messy and unbalanced.

somiaj · 2024-11-11T01:10:04Z

@drgrice1 I'm okay with it not being there, I also though about adding a comment field that could be used if adding the user to the admin course. But I'm fine with just removing it. I mostly didn't like the old behavior of making the studentID = userID, I would rather just leave it blank.

Alex-Jordan · 2024-11-11T01:16:56Z

I agree that having studentID default to userID is undesirable.

Also I rarely actually know the "studentID" of other instructors and I would not be able to enter it here, and would just leave it blank.

Not to explode the scope of this, but perhaps this whole device (where new instructors are added to a new course) could use a makeover. Why only be able to add one new user at this place? It's not uncommon to have two instructors, or one plus one or more ta~s. There could be something closer to what we see when adding a new user to a course using the Accounts Manger, all in one row, with the potential to click a button to add another row for a second additional user.

somiaj · 2024-11-11T01:20:37Z

@Alex-Jordan this PR does make a way to do that, they just have to first add multiple users in the Accounts Manager page, then just select them. Might not cover the solution where the admin doesn't want users in the admin course. I'm unsure if having a way to add more rows would be overall worth it on this page or not.

somiaj · 2024-11-11T01:31:28Z

I removed it, I too thought it looked odd, but thought maybe someone would want to use it. In general we don't have to do everything here, the accounts manager can be used to adjust things beyond what this page initially sets.

drgrice1 · 2024-11-11T02:06:48Z

I also don't like the student ID being set to the user ID. I think it would be fine just to leave it unset here.

I also agree that being able to add multiple users might be nice. That can be done later though.

templates/ContentGenerator/CourseAdmin/add_course_form.html.ep

drgrice1

Other than the style issues that I commented on, this looks good.

somiaj · 2024-11-13T07:40:49Z

Okay, I have updated it so more than a single initial user can be added. There is now a button that can be used to add another instructor to the course, which extends the form. I have also added the ability to select the permission level
of the instructor, so TAs, etc can also be added this way (to match that TAs, etc can be copied from the admin course).

In doing this I noticed a small oversight. @Alex-Jordan had set it up so initial instructors would be assigned to sets and achievements if copping those over. Since now the instructor could be copied over, that would have skipped this, also there is now more than a single instructor that can be added. Instead I updated the logic to assign all sets and achievements to any user that is not an admin (as that appeared to be the original intention, since admins were being excluded from this).

If you would prefer I move this change to another PR, let me know, I don't have to included it here.

somiaj · 2024-11-13T07:53:25Z

Two things I'm unsure about, first is permission levels. I have hard coded that professor = 10, which I should probably avoid, but unsure how to best deal with this (what if someone renames the professor role). I do see that some places we do kinda assume that some roles are left alone, but not sure the best approach. But for admin's, I actually just check for the admin role, what if someone changes that?

Second, the action is written to the log file. It use to contain the first, last, and email of the new instructor. But now these are now optional and there could be multiple new instructors either copied from the admin course directly or created. I just went with not included that info in the log file. Unsure if I should list all the user IDs created and/or copied to the course or not.

somiaj · 2024-11-13T14:01:25Z

In testing this I notice another thing I don't know how to work around (maybe it is not possible). So when adding users and the "add another instructor" or an error occurs, any passwords are not saved and have to be reentered. I think this is probably desirable behavior, but it makes the idea of entering one user's info at a time then hitting 'add another instructor' not as convenient if you have to wait until the end to enter all their passwords.

drgrice1 · 2024-11-13T14:13:30Z

Unfortunately I don't think that there is a work around. Browsers are rather restrictive with password fields.

I think assigning sets to all but admin users should be fine. Although, we could just make it assign sets to all users. Is there any reason not to assign sets to admin users? Often admin users need to access sets to deal with issues, so it may be more convenient to just do that.

somiaj · 2024-11-13T14:17:11Z

I thought the same thing, and I am often the only instructor and an admin user, so I wouldn't have sets assigned to me. I'll wait for @Alex-Jordan to respond, as I was just following what appeared to be their intent with only assigning sets to the newly created user.

somiaj · 2024-11-13T20:06:26Z

@Alex-Jordan Okay I think I fixed it. I also made it so all users (including admin users) now get assigned to everything when copying over the new course (I can remove admins from this if you would prefer).

One thing about the approach I went with, is if a user is being copied or added from the admin course, this means that users is assigned to every set and every achievement, and the sets/achievements they were assigned to in the old course is ignored.

Alex-Jordan · 2024-11-13T20:11:30Z

That sounds good but I do think that this could not be done for admin users. I have some faculty who prefer to not see the admin users show up in the scoring tools output, and not assigning them assignments prevents that.

Does it also prevent that for "observer" users? If so, making sure they are added as observers would also be OK. Maybe better, since as an admin user helping someone in their course, it can frustrate things to not be assigned a particular set.

somiaj · 2024-11-13T20:21:18Z

The observer status does exactly that. Observers are not included in any summary reports like scoring cvs files and the statistics page. Admin users added from the add course form have been made observers since that status was added (so 2.19 at least and maybe 2.18 too). So they are currently being made observers and nothing to do.

Note admin users will show up when they are on the page to assigns users to a set, and will get assigned new sets with assign all, but this would also be the case if they weren't assigned any sets. Since admins aren't taking quiz versions they won't show up when manually grading quizzes, but they might show up when manually grading assignments. One thing to do is put these users in a different section, and then one could filter by section if they don't want to have to skip over them if using the single problem grader. They will probably just have to skip over them if using the other problem grader.

somiaj · 2024-11-14T04:54:32Z

I played with adding a padded <hr> between users, would this look good, or think just a little more space is all that will be needed? (Note I only put the line between users, the bottom will just be the button as is)

Alex-Jordan · 2024-11-16T04:22:19Z

I am not a fan of hr in places like this. The extra space is good enough for me.

somiaj · 2024-11-16T04:49:04Z

What spacing should I increase it to? Of the two you gave, mb-4 is probably what I would go with, I think the other is a bit too spaced out, but I don't have strong opinions.

somiaj · 2024-11-17T17:46:30Z

@Alex-Jordan I added an "Enter information for additional instructor number X." to each additional instructor to clarify which inputs go to which instructor (also any errors will reference the number), which also breaks the inputs apart as well. Thoughts?

somiaj · 2024-11-20T01:08:09Z

I was looking at the add user page under accounts manager, and passwords there are just normal text fields, not plain text. Should I make that the same here for the password fields, that way when the add another user button is pressed, the passwords are still there? Or should we also make the password fields passwords on when adding users in a normal course?

drgrice1 · 2024-11-20T02:29:13Z

I think that the best way to go is to make this page more like the add users page for the accounts manager.

Generally, a masked password field is for when you are creating a password for yourself, or at least for one user. You really wouldn't want that for a long list of users. The thing is that a masked password field needs to have an accompanying masked confirm password field. That is really the point of the confirmation fields. Since the password field is masked, you need to ensure that the created password is typed correctly. You might have a typo when you type it the first time, and then if that is used you would get locked out of your account. But it is unlikely you would have the same typo twice. You really don't want to have to type the password and a confirmation password for each user in a long list of users. Then if you have a typo in one and submit, you have to start all over again.

If you are editing a long list of users you should be doing that somewhere where you won't have anyone looking over your shoulder.

As such, that is probably what needs to be done here. Say you add ten users when creating a course. You now have to enter ten passwords twice with no typos.

drgrice1 · 2024-11-20T02:44:07Z

lib/WeBWorK/ContentGenerator/CourseAdmin.pm

+		my $PermissionLevel = $db->getPermissionLevel($userID);
+		my $User            = $db->getUser($userID);
+		my $Password        = $db->getPassword($userID);
+		$User->status('O');    # Add admin course user as an observer.


I think this should not set the status to observer for student users. I just tested this and had created a student user in the admin course, and copied that user to the newly created course. I saw that the student was added as an observer. I don't think that should be. Of course, usually student users should not be added in this way, but it is something that could be done.

This change does not need to be made for this pull request. It is just something that I observed.

I still approve this pull request.

somiaj · 2024-11-20T04:41:50Z

Updates. Password field for new users is now text so this is saved between loads such as hitting add another user or an error. Dropped the confirm password and student ID fields, this makes each user take up a bit less space.

I also made it so if student users are added on this page, they are enrolled in the course as current, while all other non-student users are observers.

When adding a course on the course admin page, list all users in the admin course and let the admin select which users to copy over to the new course (by default all admin users are selected). This way an admin can select an existing user to copy over which can include any password or OTP secrets setup for that use (currently the admin will have to copy that over to the admin course manually). This also allows only copying a subset of admins to a newly created course instead of all. When creating a new user to add to a course, be consistent with adding a user on the accounts management page, which doesn't require a password (useful when using external auth), email, etc. Now the only required field is the new user ID. Flag the new user password input as 'new-password', so webbrowsers don't try to auto fill it in. Add the student ID as a field they can fill out instead of making it equal to the user ID. Since users can be copied from the admin course, add an option to add the new user to the admin course (as a dropped user so they can't login) so admins can select the same user to add to future courses as needed. Remove the empty list of sets to assign users to when adding new users on the accounts management page in the admin course. Make it so all users are shown on the Email page in the admin course, since most instructors are "Dropped" to still allow sending them emails.

This way it is harder to unselect the admin users, to avoid accidentally unselected them if ctrl isn't being held down.

Use the course_admin_id instead of 'admin' when talking about the admin course name on this page and in its help. Mention setting $permissionLevels{login}='admin' in course.conf in the help as a way to control access to the admin course. Use `overflow-auto` and `max-height` in the select which admin users to add to the new course div.

When adding a new course, and someone unchecks all admin users to be copied to the new course, ensure they stay unchecked if an error occurs and the page is reloaded.

When adding a new course, there is now an 'Add Another Instructor' button which can be used to add more than one new user to the newly created course. Each time this button is hit, the form expands allowing input for a new user. It is possible to set the permission level of each of these users and to keep the form balanced, the student ID can now also be included. This also updates assigning the initial user to sets and achievements. The intention was to assign new instructors to these, so any user (either copied from the admin course or added initially) whose permission is less than admin will be assigned to the sets and achievements.

When copying over old sets and achievements only assign the initial users to sets or achievements once. This makes it so any initial user is always assigned to everything, and will ignore (skip over) whatever they were assigned to in the old course. Also assign all initial users (including admins) to sets/achievements.

Add an "Enter information for additional instructor number X." statement for each additional instructor to clarify what inputs go with what instructor, and to break the inputs apart a little better.

* Remove statements about Course ID and User ID restrictions and turn them into tooltips. * Limit Course ID to 40 characters. * Change language from "instructor" to "user" with other language tweaks. * Make "Add Additional User" button secondary. * Update add course help.

Make password fields for new users text, so they will be saved after hitting add another user or errors appear. Since they are passwords for other users no need to hide them. Remove confirm password and student ID input to make each new user take up a little less space. Also make it so new student users are current (enrolled) in the course, while all other users are observers.

drgrice1 · 2024-11-24T17:20:15Z

lib/WeBWorK/ContentGenerator/CourseAdmin.pm

+	for (1 .. $number_of_additional_users) {
+		my $userID = trim_spaces($c->param("add_initial_userID_$_")) || '';
+
+		unless ($userID =~ /^[\w-.,]*$/) {


I missed this before. This is an invalid regular expression. The - inside the braces needs to be at the end of all other characters. Otherwise it is interpreted as a range, and the range from \w to . is not valid.

Note the - can also be at the beginning.

Or it can be escaped with a \.

Thanks for spotting this.

somiaj force-pushed the admin-add-users-to-new-course branch from 1dc6f5a to ce2d50a Compare November 9, 2024 19:49

Alex-Jordan reviewed Nov 10, 2024

View reviewed changes

drgrice1 reviewed Nov 11, 2024

View reviewed changes

templates/ContentGenerator/CourseAdmin/add_course_form.html.ep Outdated Show resolved Hide resolved

somiaj force-pushed the admin-add-users-to-new-course branch from faa7fe0 to 6d39ef4 Compare November 11, 2024 00:46

somiaj force-pushed the admin-add-users-to-new-course branch from 6d39ef4 to 864cd8f Compare November 11, 2024 00:55

drgrice1 reviewed Nov 11, 2024

View reviewed changes

templates/ContentGenerator/CourseAdmin/add_course_form.html.ep Outdated Show resolved Hide resolved

drgrice1 reviewed Nov 11, 2024

View reviewed changes

templates/ContentGenerator/CourseAdmin/add_course_form.html.ep Outdated Show resolved Hide resolved

drgrice1 approved these changes Nov 11, 2024

View reviewed changes

somiaj force-pushed the admin-add-users-to-new-course branch from b21e46c to 93baf98 Compare November 13, 2024 07:36

somiaj force-pushed the admin-add-users-to-new-course branch from 93baf98 to 93eac61 Compare November 13, 2024 07:44

somiaj force-pushed the admin-add-users-to-new-course branch from 93eac61 to 84d0a3b Compare November 13, 2024 13:58

somiaj force-pushed the admin-add-users-to-new-course branch from 84d0a3b to 3fbc7de Compare November 13, 2024 14:06

somiaj force-pushed the admin-add-users-to-new-course branch from 3fbc7de to 4f49c7a Compare November 13, 2024 14:14

somiaj mentioned this pull request Nov 14, 2024

Hotfix: Prevent error when adding new course. #2623

Merged

somiaj force-pushed the admin-add-users-to-new-course branch from f750bb0 to b524a75 Compare November 17, 2024 17:45

drgrice1 reviewed Nov 20, 2024

View reviewed changes

somiaj added 12 commits November 22, 2024 18:41

Use checkbox list instead of select for admin users.

07e3a65

This way it is harder to unselect the admin users, to avoid accidentally unselected them if ctrl isn't being held down.

Ensure all admin users stay unchecked after error.

beb78ea

When adding a new course, and someone unchecks all admin users to be copied to the new course, ensure they stay unchecked if an error occurs and the page is reloaded.

Remove student ID input from add course page.

032c304

Tweaks to add course form layout.

0a19eed

Update langauge for copying admin users as suggsted in the gh review.

1a0169f

Add statement for each additional instructor on add course form.

b10aee4

Add an "Enter information for additional instructor number X." statement for each additional instructor to clarify what inputs go with what instructor, and to break the inputs apart a little better.

somiaj force-pushed the admin-add-users-to-new-course branch from c8701cb to 52a38d3 Compare November 23, 2024 01:41

Alex-Jordan merged commit 8fa8546 into openwebwork:develop Nov 24, 2024
2 checks passed

drgrice1 reviewed Nov 24, 2024

View reviewed changes

somiaj mentioned this pull request Nov 24, 2024

Fix invalid regex by moving hyphen to last character. #2637

Merged

somiaj deleted the admin-add-users-to-new-course branch November 24, 2024 19:02

Updates to adding users to newly created courses. #2619

Updates to adding users to newly created courses. #2619

Conversation

somiaj commented Nov 9, 2024

Alex-Jordan commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alex-Jordan commented Nov 10, 2024

somiaj commented Nov 11, 2024

Choose a reason for hiding this comment

drgrice1 commented Nov 11, 2024

somiaj commented Nov 11, 2024

drgrice1 commented Nov 11, 2024

somiaj commented Nov 11, 2024

Alex-Jordan commented Nov 11, 2024

somiaj commented Nov 11, 2024

somiaj commented Nov 11, 2024

drgrice1 commented Nov 11, 2024

drgrice1 left a comment

Choose a reason for hiding this comment

somiaj commented Nov 13, 2024

somiaj commented Nov 13, 2024

somiaj commented Nov 13, 2024

drgrice1 commented Nov 13, 2024

somiaj commented Nov 13, 2024

somiaj commented Nov 13, 2024

Alex-Jordan commented Nov 13, 2024

somiaj commented Nov 13, 2024

somiaj commented Nov 14, 2024

Alex-Jordan commented Nov 16, 2024

somiaj commented Nov 16, 2024

somiaj commented Nov 17, 2024

somiaj commented Nov 20, 2024

drgrice1 commented Nov 20, 2024

Choose a reason for hiding this comment

somiaj commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment