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

Refactor instructor privilege APIs to reduce granularity #11458

Closed
wkurniawan07 opened this issue Oct 24, 2021 · 0 comments · Fixed by #11459
Closed

Refactor instructor privilege APIs to reduce granularity #11458

wkurniawan07 opened this issue Oct 24, 2021 · 0 comments · Fixed by #11459
Assignees
Labels
a-Scalability Behaviour at increasing/decreasing loads c.Task Other non-user-facing works, e.g. refactoring, adding tests p.Urgent Would like to handle in the very next release

Comments

@wkurniawan07
Copy link
Member

wkurniawan07 commented Oct 24, 2021

Current: the instructor privilege-related APIs work in a granular way, e.g. only fetch/update course-level, section-level, or session-level privileges depending on the query parameters passed.

String sectionName = getRequestParamValue(Const.ParamsNames.SECTION_NAME);
String feedbackSessionName = getRequestParamValue(Const.ParamsNames.FEEDBACK_SESSION_NAME);
InstructorPrivilegeData response = constructInstructorPrivileges(instructor, feedbackSessionName);
if (sectionName != null) {
response.constructSectionLevelPrivilege(instructor.getPrivileges(), sectionName);
if (feedbackSessionName != null) {
response.constructSessionLevelPrivilege(instructor.getPrivileges(), sectionName, feedbackSessionName);
}
}

String sectionName = request.getSectionName();
String sessionName = request.getFeedbackSessionName();
Map<String, Boolean> courseLevelPrivilegesMap = request.getAllPresentCourseLevelPrivileges();
Map<String, Boolean> sectionLevelPrivilegesMap = request.getAllPresentSectionLevelPrivileges();
Map<String, Boolean> sessionLevelPrivilegesMap = request.getAllPresentSessionLevelPrivileges();
if (sectionName == null && sessionName == null) {
updateCourseLevelPrivileges(courseLevelPrivilegesMap, instructorToUpdate);
updateCourseLevelPrivileges(sectionLevelPrivilegesMap, instructorToUpdate);
updateCourseLevelPrivileges(sessionLevelPrivilegesMap, instructorToUpdate);
} else if (sessionName == null) {
updateSectionLevelPrivileges(sectionName, sectionLevelPrivilegesMap, instructorToUpdate);
updateSectionLevelPrivileges(sectionName, sessionLevelPrivilegesMap, instructorToUpdate);
} else {
updateSessionLevelPrivileges(sectionName, sessionName, sessionLevelPrivilegesMap, instructorToUpdate);
}

Problems:

  • The storage does not match the granularity. The instructor privilege is always saved as one string representing all the instructor's allowed actions, thus fetching/updating part of the privilege have the same DB read/write cost as fetching/updating everything.
  • The granularity caused unnecessary burst of requests (both the number and the rate, but especially the rate) for courses where there are large number of sections and sessions. Add that to the fact that each fetch/update operates on the same chunk of data (not part of), and it is not difficult to see the extent of wasted operations we have been doing.
    this.allSections.forEach((sectionName: string) => {
    const sectionLevelPermission: InstructorSectionLevelPermission = {
    sectionNames: [sectionName],
    privilege: {
    canModifyCourse: false,
    canModifySession: false,
    canModifyStudent: false,
    canModifyInstructor: false,
    canViewStudentInSections: false,
    canModifySessionCommentsInSections: false,
    canViewSessionInSections: false,
    canSubmitSessionInSections: false,
    },
    sessionLevel: [],
    };
    permission.sectionLevel.push(sectionLevelPermission);
    requests.push(this.instructorService.loadInstructorPrivilege({
    sectionName,
    courseId: instructor.courseId,
    instructorEmail: instructor.email,
    }).pipe(tap((resp: InstructorPrivilege) => {
    sectionLevelPermission.privilege.canViewStudentInSections = resp.canViewStudentInSections;
    sectionLevelPermission.privilege.canModifySessionCommentsInSections = resp.canModifySessionCommentsInSections;
    sectionLevelPermission.privilege.canViewSessionInSections = resp.canViewSessionInSections;
    sectionLevelPermission.privilege.canSubmitSessionInSections = resp.canSubmitSessionInSections;
    })));
    this.allSessions.forEach((sessionName: string) => {
    requests.push(this.instructorService.loadInstructorPrivilege({
    sectionName,
    feedbackSessionName: sessionName,
    courseId: instructor.courseId,
    instructorEmail: instructor.email,
    }).pipe(tap((resp: InstructorPrivilege) => {
    sectionLevelPermission.sessionLevel.push({
    sessionName,
    privilege: {
    canModifyCourse: false,
    canModifySession: false,
    canModifyStudent: false,
    canModifyInstructor: false,
    canViewStudentInSections: false,
    canModifySessionCommentsInSections: resp.canModifySessionCommentsInSections,
    canViewSessionInSections: resp.canViewSessionInSections,
    canSubmitSessionInSections: resp.canSubmitSessionInSections,
    },
    });
    })));
    });
    });

    permission.sectionLevel.forEach((sectionLevel: InstructorSectionLevelPermission) => {
    sectionLevel.sectionNames.forEach((sectionName: string) => {
    requests.push(
    this.instructorService.updateInstructorPrivilege({
    courseId: instructor.courseId,
    instructorEmail: instructor.email,
    requestBody: {
    sectionName,
    canViewStudentInSections: sectionLevel.privilege.canViewStudentInSections,
    canModifySessionCommentsInSections: sectionLevel.privilege.canModifySessionCommentsInSections,
    canViewSessionInSections: sectionLevel.privilege.canViewSessionInSections,
    canSubmitSessionInSections: sectionLevel.privilege.canSubmitSessionInSections,
    } as InstructorPrivilegeUpdateRequest,
    }).pipe(tap((resp: InstructorPrivilege) => {
    sectionLevel.privilege.canViewStudentInSections = resp.canViewStudentInSections;
    sectionLevel.privilege.canModifySessionCommentsInSections = resp.canModifySessionCommentsInSections;
    sectionLevel.privilege.canViewSessionInSections = resp.canViewSessionInSections;
    sectionLevel.privilege.canSubmitSessionInSections = resp.canSubmitSessionInSections;
    })));
    sectionLevel.sessionLevel.forEach((sessionLevel: InstructorSessionLevelPermission) => {
    requests.push(
    this.instructorService.updateInstructorPrivilege({
    courseId: instructor.courseId,
    instructorEmail: instructor.email,
    requestBody: {
    sectionName,
    feedbackSessionName: sessionLevel.sessionName,
    canModifySessionCommentsInSections: sessionLevel.privilege.canModifySessionCommentsInSections,
    canViewSessionInSections: sessionLevel.privilege.canViewSessionInSections,
    canSubmitSessionInSections: sessionLevel.privilege.canSubmitSessionInSections,
    } as InstructorPrivilegeUpdateRequest,
    }).pipe(tap((resp: InstructorPrivilege) => {
    sessionLevel.privilege.canModifySessionCommentsInSections = resp.canModifySessionCommentsInSections;
    sessionLevel.privilege.canViewSessionInSections = resp.canViewSessionInSections;
    sessionLevel.privilege.canSubmitSessionInSections = resp.canSubmitSessionInSections;
    })));
    });
    });
    });

Solution:
Simplify the APIs to always fetch/update the entire permission object (thus the update behaving more like a PUT than a PATCH).

@wkurniawan07 wkurniawan07 added a-Scalability Behaviour at increasing/decreasing loads c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Oct 24, 2021
@wkurniawan07 wkurniawan07 self-assigned this Oct 24, 2021
@wkurniawan07 wkurniawan07 added the p.Urgent Would like to handle in the very next release label Oct 24, 2021
wkurniawan07 added a commit that referenced this issue Nov 21, 2021
…1459)

* Rename getTextFromInstructorPrivileges to getInstructorPrivilegesAsText

* Add proper object representation for instructor permission set

* Add construct for default instructor permissions

* Reuse InstructorPrivileges data structure in the API output/request

* Fix test data

* Adapt front-end for new API output/input format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Scalability Behaviour at increasing/decreasing loads c.Task Other non-user-facing works, e.g. refactoring, adding tests p.Urgent Would like to handle in the very next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant