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

RepoCreatorService: Create custom operators for permissions request #671

Merged
merged 23 commits into from
Apr 5, 2021
Merged

RepoCreatorService: Create custom operators for permissions request #671

merged 23 commits into from
Apr 5, 2021

Conversation

seanlowjk
Copy link
Contributor

@seanlowjk seanlowjk commented Mar 26, 2021

Summary

This PR fixes #549 by completing the last task of the issue. which abstracts the repo creation permissions request step of sessionSetup() in PhaseService to a single, readable custom operator in RepoCreatorService.

Description

This PR does the following:

  1. requestRepoCreationPermissions() takes in two arguments, currentPhase and phaseRepo and checks for the following:
    a. if isSessionAvailable is false and if the currentPhase is Phase.phaseBugReporting, it suggests that there is a need for request of repo creation permissions
    b. Else, no request for permissions is needed.

Suggested Commit Message

RepoCreatorService: Create custom operators for Permissions Request

To improve readability of the session setup code, let's shift the 
request for repo creation permissions code into a custom operator
in RepoCreatorService
  
Let's also add tests for this custom operator.

@damithc
Copy link
Contributor

damithc commented Mar 26, 2021

Suggested Commit Message

RepoCreatorService: Create custom operators for Requesting Repo Creation Permissions

To improve readability of the session setup code,
let's shift the request for repo creation permissions code 
into one custom operators in  RepoCreatorService

Let's also add tests for these custom operator.

Subject seems too long (?) and the body width seems too narrow https://se-education.org/guides/conventions/git.html

@seanlowjk seanlowjk changed the title RepoCreatorService: Create custom operators for Requesting Repo Creation Permissions RepoCreatorService: Create custom operators for Permissions Request Mar 26, 2021
@seanlowjk
Copy link
Contributor Author

Thanks Prof @damithc for the link. I have rectified the subject of the PR and the commit message as followed.

@codecov-io
Copy link

codecov-io commented Mar 26, 2021

Codecov Report

Merging #671 (ac77dda) into master (01ce27e) will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   68.38%   68.67%   +0.29%     
==========================================
  Files          75       75              
  Lines        2274     2276       +2     
  Branches      208      207       -1     
==========================================
+ Hits         1555     1563       +8     
+ Misses        675      668       -7     
- Partials       44       45       +1     
Impacted Files Coverage Δ
src/app/core/services/phase.service.ts 73.01% <ø> (+6.34%) ⬆️
src/app/core/services/repo-creator.service.ts 100.00% <100.00%> (ø)
src/app/shared/issue-tables/issue-sorter.ts 44.00% <0.00%> (-4.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01ce27e...ac77dda. Read the comment docs.

@seanlowjk seanlowjk requested a review from a team March 26, 2021 12:12
@seanlowjk seanlowjk marked this pull request as ready for review March 26, 2021 12:12
Copy link
Contributor

@ptvrajsk ptvrajsk left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me 👍 Just some minor queries regarding the test description but great job otherwise 👍

tests/services/repo-creator.service.spec.ts Outdated Show resolved Hide resolved
tests/services/repo-creator.service.spec.ts Outdated Show resolved Hide resolved
src/app/core/services/phase.service.ts Show resolved Hide resolved
src/app/core/services/repo-creator.service.ts Outdated Show resolved Hide resolved
tests/services/repo-creator.service.spec.ts Outdated Show resolved Hide resolved
src/app/core/services/repo-creator.service.ts Outdated Show resolved Hide resolved
* @return Observable<boolean> - Representing user's permission grant.
*/
private openSessionFixConfirmation(phaseRepo: string): Observable<boolean> {
const dialogRef: MatDialogRef<SessionFixConfirmationComponent> =
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename the SessionFixConfirmationComponent --> RepoCreationConfirmationComponent too.
In my view, "RepoCreation" has a more obvious meaning than "SessionFix".
What do you think?

This renaming can be done separately in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would be better to do so!

I think we can do this in a separate PR as well 😄

src/app/core/services/repo-creator.service.ts Outdated Show resolved Hide resolved
Co-authored-by: Anubhav <35621759+anubh-v@users.noreply.github.com>
src/app/core/services/repo-creator.service.ts Outdated Show resolved Hide resolved
@anubh-v anubh-v changed the title RepoCreatorService: Create custom operators for Permissions Request RepoCreatorService: Create custom operators for permissions request Apr 2, 2021
@anubh-v anubh-v merged commit 5ba93d4 into CATcher-org:master Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PhaseService: Improve readability of session setup logic (using custom operators)
5 participants