Fix part of issue #19435: Migrate Skill Editor Page #19882
Conversation
|
Assigning @seanlip for the first pass review of this PR. Thanks! |
|
PTAL @DubeySandeep |
|
Hi @shivanandan17. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
|
Assigning @Nik-09 for code owner reviews. Thanks! |
| ); | ||
| }); | ||
|
|
||
| it('should allow user to access the page if user is curriculum admin', |
There was a problem hiding this comment.
This doesn't depend on user role, right? (as it depends on accessValidatorfunction)
There was a problem hiding this comment.
Done. (Updated the File)
| const userInfo = await this.userService.getUserInfoAsync(); | ||
| if (userInfo.isCurriculumAdmin()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Are we expecting to use the validateAccessToSkillEditorPage here?
There was a problem hiding this comment.
Done. (Updated the File)
|
Unassigning @DubeySandeep since they have already approved the PR. |
|
@shivanandan17 Tests are still failing on this PR, PTAL! (We need to get this merged quickly!) |
|
Hi @shivanandan17. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
shivanandan17
force-pushed
the
migrate-skill-editor
branch
from
1aeb78b
to
e0a3e9e
Compare
| let canActivateResult: boolean | null = null; | ||
|
|
||
| guard | ||
| .canActivate(new ActivatedRouteSnapshot(), {} as RouterStateSnapshot) | ||
| .then(result => { | ||
| canActivateResult = result; | ||
| }); |
There was a problem hiding this comment.
Can't we fit all this in one statement?
const resultCanBeActivated = guard.canActivate(
new ActivatedRouteSnapshot(),
{} as RouterStateSnapshot
).then(result => result);Ditto for below.
| self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME) | ||
| self.login(self.VIEWER_EMAIL) | ||
| self.get_json( | ||
| '%s/can_access_skill_editor/%s' % ( | ||
| ACCESS_VALIDATION_HANDLER_PREFIX, self.skill_id | ||
| ), expected_status_int=401 | ||
| ) | ||
| self.logout() | ||
|
|
||
| self.login(self.NEW_USER_EMAIL) | ||
| self.get_json( | ||
| '%s/can_access_skill_editor/%s' % ( | ||
| ACCESS_VALIDATION_HANDLER_PREFIX, self.skill_id | ||
| ), expected_status_int=401 | ||
| ) | ||
| self.logout() |
There was a problem hiding this comment.
What's the difference between a viewer and a new user? Because either:
- There are discernible differences between both user roles, so they should be in their own separate test OR
- There is no difference between both user roles, so we should drop one of these group of statements to avoid exercising duplicate behaviors
|
Hi @shivanandan17, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
|
Hi @shivanandan17, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
|
Hi @shivanandan17, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Overview
Essential Checklist
Proof that changes are correct
12e44861-adab-41b5-ac2e-b79ddec86556_hrDStOhj.mp4
PR Pointers