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

[#11362] [#11617] Use course institute fully for displays + to determine whether account can create course #11654

Merged
merged 13 commits into from
Apr 6, 2022
Merged

[#11362] [#11617] Use course institute fully for displays + to determine whether account can create course #11654

merged 13 commits into from
Apr 6, 2022

Conversation

wkurniawan07
Copy link
Member

@wkurniawan07 wkurniawan07 commented Mar 18, 2022 •
edited
Loading

Fixes #11362
Fixes #11617

Description

  • This is the grand finale of migrating the institute field from Account to Course.
  • As institute in account is the source of truth when determining the institute of a new course, removing the said field necessitates a new strategy to get the institute field. However, by a stroke of luck, the identified new strategy happens to be the one that solves another issue.
  • We also get a bonus by successfully identifying and removing another now-unused field: isInstructor.

Outline of Solution

  • In all places where institute is obtained from account, they are now obtained from course instead. This applies to a few places:
    • Script that calculates usage statistics per institute. This greatly simplifies the said script.
    • Removal of institute name in the footer.
    • Additional of institute name in selected few pages, where institute name is determined to have value-add: admin account page, course details page, and submission/results page.
      • The latter incurs additional API call to get the course information. However, this is offset by the removal of API call that populates the (formerly) institute at the footer.
      • GET /course API needs to be opened to unregistered users (with key access), which is not an issue as there is no sensitive information that necessitates logging in.
  • The institute for a newly created course is now determined via request parameter.
    • The institute will be validated against a list of institutes in which the user has a co-owned course in. Thus, this serves as an effective filter on who can create new courses (most certainly, not tutors!).
  • It is identified that account.isInstructor field is only used in the previous operation and for checking whether a user should be allowed access to instructor pages (in the front-end).
    • The latter is easily replaceable by checking whether there are instructor entities for the account, i.e. the same way of checking whether a user should be allowed access to student pages.
    • After replacing the above, account.isInstructor has no more use and the field is removed. As a result, there is no more distinction between "instructor account" and "student account", and "downgrading an account" stops making sense.

Screenshots

  • Course details page (shown here is the instructor; student is similar)
    Screenshot 2022-03-19 at 6 47 48 PM
  • Submission page (result page is similar)
    Screenshot 2022-03-19 at 6 48 41 PM
  • Admin account page
    Screenshot 2022-03-19 at 6 50 46 PM
  • Course add page
    Screenshot 2022-03-19 at 6 51 14 PM
  • If user adds a course to an unauthorized institute
    Screenshot 2022-03-19 at 6 57 02 PM

@wkurniawan07 wkurniawan07 mentioned this pull request Mar 18, 2022
Closed
@wkurniawan07 wkurniawan07 force-pushed the remove-institute-from-account branch 4 times, most recently from bf4b61c to 3c6d349 Compare March 19, 2022 11:23
@wkurniawan07 wkurniawan07 mentioned this pull request Mar 19, 2022
Closed
5 tasks
@madanalogy madanalogy added the s.Ongoing The PR is being worked on by the author(s) label Mar 22, 2022
@wkurniawan07 wkurniawan07 force-pushed the remove-institute-from-account branch 3 times, most recently from 8de5483 to fb54cb6 Compare March 28, 2022 17:32
@wkurniawan07 wkurniawan07 added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 30, 2022
@wkurniawan07 wkurniawan07 marked this pull request as ready for review March 30, 2022 10:54
samuelfangjw
samuelfangjw reviewed Mar 30, 2022
View reviewed changes
Copy link
Member

@samuelfangjw samuelfangjw left a comment

Choose a reason for hiding this comment

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

I didn't find much else other than the issue on the course page. Some comments below!

src/main/java/teammates/common/datatransfer/attributes/AccountAttributes.java Outdated Show resolved Hide resolved
src/main/java/teammates/storage/api/InstructorsDb.java Outdated Show resolved Hide resolved
src/web/services/course.service.ts Show resolved Hide resolved
src/web/app/pages-session/session-submission-page/session-submission-page.component.ts Outdated Show resolved Hide resolved
@wkurniawan07 wkurniawan07 force-pushed the remove-institute-from-account branch 2 times, most recently from ada7965 to b2948b5 Compare March 31, 2022 18:50
@wkurniawan07 wkurniawan07 added this to the V8.13.0 milestone Apr 2, 2022
@wkurniawan07 wkurniawan07 force-pushed the remove-institute-from-account branch 3 times, most recently from e10189a to 70a679b Compare April 4, 2022 01:39
@samuelfangjw
Copy link
Member

@wkurniawan07, I seem to be facing the same issue.

Steps to replicate:

  1. Started with new clean datastore
  2. Add myself as instructor
  3. Unable to add course :(

Screen Shot 2022-04-04 at 11 26 45 PM

@wkurniawan07
Copy link
Member Author

@samuelfangjw really sorry to say this, but I'm almost certain something is amiss with your setup. One of the E2E tests would have guaranteed that this should not happen.

@samuelfangjw
Copy link
Member

@samuelfangjw really sorry to say this, but I'm almost certain something is amiss with your setup. One of the E2E tests would have guaranteed that this should not happen.

Did a little digging, I realized there are two ways to ways to add courses that behave differently for me.

  1. Adding from home page. This doesn't work for me. I don't believe this path is covered by e2e tests, though I might be mistaken.
    Home Page -> Add New Course
  2. Go to courses page, open the panel to add course. This works as expected for me.

Can I check if 1 works as expected for you?

@wkurniawan07 wkurniawan07 force-pushed the remove-institute-from-account branch from 70a679b to 77f873d Compare April 5, 2022 00:30
samuelfangjw
samuelfangjw approved these changes Apr 5, 2022
View reviewed changes
Copy link
Member

@samuelfangjw samuelfangjw left a comment

Choose a reason for hiding this comment

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

LGTM!

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Apr 5, 2022
@wkurniawan07 wkurniawan07 added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Apr 5, 2022
@wkurniawan07
Use course as the source of institute instead of account
1f97bab
@wkurniawan07
Remove institute information from footer
4ca1625
@wkurniawan07
Remove account institute information in admin account page
bdd8d6b
@wkurniawan07
Display course institute information in admin account page
7479113
@wkurniawan07
Display course institute in instructor/student course details page
cdff428
@wkurniawan07
Extend GetCourseAction to be accessible by unregistered users
fe6a58c
@wkurniawan07
Display course institute in session submission/results page
075b217
@wkurniawan07
Remove institute from account entity
03eef44
@wkurniawan07
Check for institute validity when creating course
4a386e9
@wkurniawan07
Remove account isInstructor information in admin account page
b68727c
@wkurniawan07
Remove function to downgrade instructor in admin accounts page
6c64214
@wkurniawan07
Remove isInstructor from account entity
0361391
@wkurniawan07
Remove unused createAtTimestamp for account output
4a8df41
@wkurniawan07 wkurniawan07 force-pushed the remove-institute-from-account branch from 77f873d to 4a8df41 Compare April 6, 2022 13:42
@wkurniawan07 wkurniawan07 merged commit 6ef3fb2 into TEAMMATES:master Apr 6, 2022
@wkurniawan07 wkurniawan07 deleted the remove-institute-from-account branch April 6, 2022 14:16
ziqing26 pushed a commit to ziqing26/teammates that referenced this pull request Apr 11, 2022
@wkurniawan07 @ziqing26
[TEAMMATES#11362] [TEAMMATES#11617] Use course institute fully for di…
b309ff0 
…splays + to determine whether account can create course (TEAMMATES#11654)

* Use course as the source of institute instead of account

* Remove institute information from footer

* Remove account institute information in admin account page

* Display course institute information in admin account page

* Display course institute in instructor/student course details page

* Extend GetCourseAction to be accessible by unregistered users

* Display course institute in session submission/results page

* Remove institute from account entity

* Check for institute validity when creating course

* Remove account isInstructor information in admin account page

* Remove function to downgrade instructor in admin accounts page

* Remove isInstructor from account entity

* Remove unused createAtTimestamp for account output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelfangjw samuelfangjw samuelfangjw approved these changes

Assignees
No one assigned
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V8.13.0
Development

Successfully merging this pull request may close these issues.

Restrict course creation only to users who requested accounts Move institute field to Course object
3 participants
@wkurniawan07 @samuelfangjw @madanalogy