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.

Sign up for GitHub

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

Jump to bottom

Add a setting to courses to limit registration to institutional accounts #3752

Merged
merged 17 commits into from
Aug 16, 2022
Merged

Add a setting to courses to limit registration to institutional accounts #3752

merged 17 commits into from
Aug 16, 2022

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Jun 29, 2022 •
edited
Loading

This pull request adds a setting to courses to limit registration to institutional accounts. (Thus excluding personal accounts). This is also the new default setting for courses.

image

What a personal account sees:
image

  • Tests were added

Part of #2597
Closes #3802

@jorg-vr jorg-vr self-assigned this Jun 29, 2022
@jorg-vr jorg-vr changed the title Add a setting to courses to allow registration with personal accounts Add a setting to courses to limit registration and/or visibility to institutional accounts Jun 30, 2022
@jorg-vr jorg-vr marked this pull request as ready for review June 30, 2022 13:02
@jorg-vr jorg-vr requested a review from a team as a code owner June 30, 2022 13:02
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team June 30, 2022 13:02
@jorg-vr jorg-vr added the feature New feature or request label Jul 1, 2022
@jorg-vr jorg-vr marked this pull request as draft July 8, 2022 13:21
@jorg-vr jorg-vr mentioned this pull request Jul 12, 2022
Closed
1 task
@jorg-vr jorg-vr changed the title Add a setting to courses to limit registration and/or visibility to institutional accounts Add a setting to courses to limit registration to institutional accounts Aug 3, 2022
@jorg-vr jorg-vr marked this pull request as ready for review August 3, 2022 13:33
@jorg-vr jorg-vr changed the base branch from develop to feature/personal-accounts August 3, 2022 13:39
niknetniko
niknetniko requested changes Aug 5, 2022
View reviewed changes
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Looks good to me (I assume #3802 is nog longer needed then?).

Do we also want to migrate existing courses to "institutional only"?

config/locales/views/courses/en.yml Outdated Show resolved Hide resolved
config/locales/views/courses/en.yml Outdated Show resolved Hide resolved
config/locales/views/courses/nl.yml Outdated Show resolved Hide resolved
config/locales/views/courses/nl.yml Outdated Show resolved Hide resolved
@jorg-vr @niknetniko
Apply suggestions from code review
1a124a6 
Co-authored-by: Niko Strijbol <strijbol.niko@gmail.com>
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Aug 8, 2022

@niknetniko This is indeed a replacement of #3802 so I will close that pr once this one is approved

With the current code all existing courses which where open for all will be migrated to open for institutional because I changed the meaning of the enum value 0

@jorg-vr jorg-vr requested a review from niknetniko August 8, 2022 07:01
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Aug 12, 2022
@bmesuere bmesuere temporarily deployed to mestra August 12, 2022 14:39 Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Aug 12, 2022
bmesuere
bmesuere approved these changes Aug 12, 2022
View reviewed changes
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

I've added some text suggestions. Shouldn't there also be a migration here to change the access level in the database?

config/locales/views/courses/en.yml Outdated Show resolved Hide resolved
config/locales/views/courses/nl.yml Outdated Show resolved Hide resolved
config/locales/views/courses/nl.yml Outdated Show resolved Hide resolved
config/locales/views/courses/en.yml Outdated Show resolved Hide resolved
jorg-vr and others added 3 commits August 12, 2022 17:05
@jorg-vr @bmesuere
Update config/locales/views/courses/en.yml
429980d 
Co-authored-by: Bart Mesuere <Bart.Mesuere@UGent.be>
@jorg-vr @bmesuere
Update config/locales/views/courses/nl.yml
e611f71 
Co-authored-by: Bart Mesuere <Bart.Mesuere@UGent.be>
@jorg-vr @bmesuere
Apply suggestions from code review
0482cbc 
Co-authored-by: Bart Mesuere <Bart.Mesuere@UGent.be>
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Aug 12, 2022

@bmesuere as stated above

With the current code all existing courses which where open for all will be migrated to open for institutional because I changed the meaning of the enum value 0

@jorg-vr jorg-vr merged commit 3bfa9fc into feature/personal-accounts Aug 16, 2022
@jorg-vr jorg-vr deleted the feature/institutional-users-course-privacy branch August 16, 2022 07:13
@bmesuere bmesuere mentioned this pull request Aug 16, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niknetniko niknetniko niknetniko approved these changes

@bmesuere bmesuere bmesuere approved these changes

Assignees

@jorg-vr jorg-vr

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jorg-vr @bmesuere @niknetniko