LabelService: create custom operator for label synchronisation #590

seanlowjk · 2021-02-11T15:45:42Z

Summary

This PR addresses part of #549 , which abstracts the steps of sessionSetup() in PhaseService to readable, custom operators

Description

This PR does the following:

Abstracts the logic of syncing of labels in the application with the remote repository labels into the syncLabels() method in LabelService
Moves the error throwing as a readable step in sessionSetup() in PhaseService
Adds basic tests for syncLabels() and checkSessionCreation()

Suggested Commit Message:

LabelService: create custom operator for label synchronisation

Let us shift the  label synchronization custom operator back to LabelService to 
facilitate readability in sessioNSetup().

We cam also add tests for this custom operator: to create: 
- all required labels to be created when no required labels are synchronized
- some required labels to be created when at least 1 required label is synchronized
- no required labels to be created when all the required labels are synchronized

To achieve this, we would need to 
- Keep the error handling  operator within PhaseService 
- Create a new custom operator syncLabels() to call upon synchronizeRemoteLabels()

kkangs0226

Other than the comments I've left, it looks good to me!

src/app/core/services/repository.service.ts

kkangs0226 · 2021-02-17T23:42:55Z

tests/services/repository.service.spec.ts

+
+describe('RepositoryService', () => {
+  beforeEach(() => {
+    labelService = jasmine.createSpyObj('GithubService', ['synchronizeRemoteLabels']);


Can I just check why is it 'GithubService' for the spy object? Shouldn't it be 'labelService' instead? Let me know if my interpretation is correct😅

Oh yes i made a typo! Thanks for pointing that out!!

…anlowjk/CATcher into refactor/repo-creator-service

codecov-io · 2021-02-18T01:05:30Z

Codecov Report

Merging #590 (d59e8ce) into master (df9147b) will increase coverage by 0.50%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   65.14%   65.65%   +0.50%     
==========================================
  Files          67       67              
  Lines        2106     2111       +5     
  Branches      199      198       -1     
==========================================
+ Hits         1372     1386      +14     
+ Misses        696      686      -10     
- Partials       38       39       +1

Impacted Files	Coverage Δ
src/app/core/services/phase.service.ts	`54.54% <50.00%> (+1.67%)`	⬆️
src/app/core/services/label.service.ts	`78.02% <100.00%> (+7.90%)`	⬆️
src/app/core/models/label.model.ts	`90.00% <0.00%> (+20.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 df9147b...d59e8ce. Read the comment docs.

ptvrajsk

LGTM 👍 Just one question regarding the test case

ptvrajsk · 2021-02-18T06:05:58Z

tests/services/repository.service.spec.ts

+  describe('.syncLabels()', () => {
+    it('should throw an error given an Observable of false', () => {
+      of(false)
+        .pipe(repositoryService.syncLabels())
+        .subscribe({
+          error: (err) =>
+            expect(err).toEqual(new Error(SESSION_AVALIABILITY_FIX_FAILED)),
+        });
+    });


A quick question cause I'm not entirely sure,
if we supply an of(true) does this test fail? Or do we need to supply the next: in the .subscribe()?

So like this? Just checking cause I'm not sure haha

it('should throw an error given an Observable of false', () => { of(false) .pipe(repositoryService.syncLabels()) .subscribe({ next: () => fail(), error: (err) => expect(err).toEqual(new Error(SESSION_AVALIABILITY_FIX_FAILED)), }); });

Yea, actually I just realised with the above, it would also pass the TC if i supply a value of true 😅
We would need to supply the next to ensure that it fails!
Thanks for noticing! ><

No worries haha actually I realized we've made this mistake in other areas also 😆 I'll make an issue to clean that up

Alright! 💯

src/app/core/services/repository.service.ts

anubh-v

Thanks @seanlowjk for working on this.

I have a different opinion that the label synchronisation custom operator should reside in LabelService, and not the newly created RepositoryService.

It seems neater to keep label-related responsibilities under LabelService.
The RepositoryService can be in charge of checking whether a repo exists, asking the user for permission to create a repo, and creating the repo (using GithubService under-the-hood as you mentioned).

We can rename RepositoryService --> RepoCreatorService (other naming suggestions welcome)

What do you all think?

src/app/core/services/repository.service.ts

seanlowjk · 2021-02-18T10:48:48Z

Yea since it is mainly responsible for the creating of the repository due to the steps mentioned earlier, it would be better to use the name of RepoCreatorService!

I will shift synclabels() to the LabelService and the checking of sessionCreation in PhaseService as two separate steps!

seanlowjk · 2021-02-19T14:08:00Z

I am not sure if what I did for custom operator captures the original idea that was suggested! Would like your advice on this >< @ptvrajsk @anubh-v

src/app/core/services/phase.service.ts

src/app/core/services/repo-creator.service.ts

seanlowjk · 2021-02-20T14:32:07Z

Updated the title of the PR and the description to better suit the purpose of this PR!

src/app/core/services/label.service.ts

tests/services/phase.service.spec.ts

src/app/core/services/label.service.ts

tests/services/label.service.spec.ts

anubh-v · 2021-02-22T09:40:28Z

@seanlowjk I've edited your PR title to make it easier to understand. Let's try to use the scope: change style for titles, as it helps readers quickly understand which part of the app is being modified. (i.e. LabelService: create custom ...)
This is consistent with our guidelines for commit message titles too.

seanlowjk · 2021-02-22T15:27:56Z

@anubh-v have made the following changes! I have tested the following !

1, Tests that 15 labels to be created when fetchAllLabels returns []
2. Tests that < 15 labels to be created when fetchAllLabels return at least 1 label
3. Tests that 0 labels to be created when fetchAllLabels returns all the required labels!

Abstracted the asseting of label creation via assertLabelCreated and assertLabelNotCreated

Hope this helps in testing of the cases as mentioned earlier!

anubh-v

Looks good 👍 , just one nit

tests/services/label.service.spec.ts

anubh-v · 2021-02-24T13:19:30Z

@seanlowjk thanks for the work. Can you write up the commit message for this PR?

seanlowjk · 2021-02-24T15:26:23Z

@anubh-v I have key-ed in the suggested commit message to capture the idea of this PR! Hope this commit message is better with regards to capturing the purpose and how we can solve the issue mentioned!

seanlowjk added 5 commits February 10, 2021 22:55

Fix Current Tests based on new argument of RepositoryService

a0f0c9e

Add Implementation and tests for syncLabels() method

3d629c2

Remove unnecessary dependencies

5a1deed

Add minor documentation

9ca6ccb

Add minor documentation

3c2eeec

seanlowjk added the s.Ongoing label Feb 11, 2021

seanlowjk and others added 2 commits February 15, 2021 19:16

Merge branch 'master' into refactor/repo-creator-service

5d89cb3

Remove Unnecessary Comment

d0cb41e

seanlowjk added s.ToReview and removed s.Ongoing labels Feb 15, 2021

seanlowjk requested a review from a team February 15, 2021 11:22

kkangs0226 reviewed Feb 18, 2021

View reviewed changes

seanlowjk and others added 3 commits February 18, 2021 08:58

Fix Wrong Variable Naming for Spec File

b820306

Merge branch 'master' into refactor/repo-creator-service

bf8cf85

Merge branch 'refactor/repo-creator-service' of https://github.com/se…

5480f4b

…anlowjk/CATcher into refactor/repo-creator-service

ptvrajsk approved these changes Feb 18, 2021

View reviewed changes

Fix Ambiguous Test Case

91c8275

ptvrajsk mentioned this pull request Feb 18, 2021

Supply next: condition in TestCases that expect an error #607

Closed

anubh-v reviewed Feb 18, 2021

View reviewed changes

src/app/core/services/repository.service.ts Outdated Show resolved Hide resolved

seanlowjk and others added 6 commits February 19, 2021 12:54

Rename RepositoryService to RepoCreatorService

1eb5c33

Fix Methods and Specs

fce4049

Add Tests for syncLabels();

8411d36

Add tests for checkSessionCreation

1aab989

Merge branch 'master' into refactor/repo-creator-service

bab3274

Rename Files

2aaeaf5

anubh-v reviewed Feb 20, 2021

View reviewed changes

src/app/core/services/phase.service.ts Outdated Show resolved Hide resolved

src/app/core/services/phase.service.ts Outdated Show resolved Hide resolved

src/app/core/services/repo-creator.service.ts Outdated Show resolved Hide resolved

Make Final Changes to code

d59e8ce

seanlowjk changed the title ~~Create RepositoryService and add Implementation and Tests for syncLabels()~~ Move Implementation and Tests for syncLabels() Feb 20, 2021

seanlowjk requested a review from anubh-v February 22, 2021 08:07

anubh-v reviewed Feb 22, 2021

View reviewed changes

anubh-v changed the title ~~Move Implementation and Tests for syncLabels()~~ LabelService: create custom operator for label synchronisation Feb 22, 2021

seanlowjk added 3 commits February 22, 2021 23:09

Fix Impl and Spec Files based on final comments

e6a4150

Fix Linting

41bfa16

Fix Unnecessary export

7d36ad4

Remove test in subscriber

bf894a3

anubh-v approved these changes Feb 24, 2021

View reviewed changes

tests/services/label.service.spec.ts Outdated Show resolved Hide resolved

Merge branch 'master' into refactor/repo-creator-service

0b62aa5

anubh-v merged commit 517b037 into CATcher-org:master Feb 24, 2021

anubh-v added s.ToMerge and removed s.ToReview labels Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LabelService: create custom operator for label synchronisation #590

LabelService: create custom operator for label synchronisation #590

seanlowjk commented Feb 11, 2021 •

edited

kkangs0226 left a comment

kkangs0226 Feb 17, 2021

seanlowjk Feb 18, 2021

codecov-io commented Feb 18, 2021 •

edited

ptvrajsk left a comment

ptvrajsk Feb 18, 2021

seanlowjk Feb 18, 2021

ptvrajsk Feb 18, 2021

seanlowjk Feb 18, 2021

anubh-v left a comment

seanlowjk commented Feb 18, 2021

seanlowjk commented Feb 19, 2021

seanlowjk commented Feb 20, 2021

anubh-v commented Feb 22, 2021 •

edited

seanlowjk commented Feb 22, 2021

anubh-v left a comment

anubh-v commented Feb 24, 2021

seanlowjk commented Feb 24, 2021

LabelService: create custom operator for label synchronisation #590

LabelService: create custom operator for label synchronisation #590

Conversation

seanlowjk commented Feb 11, 2021 • edited

Summary

Description

Suggested Commit Message:

kkangs0226 left a comment

Choose a reason for hiding this comment

kkangs0226 Feb 17, 2021

Choose a reason for hiding this comment

seanlowjk Feb 18, 2021

Choose a reason for hiding this comment

codecov-io commented Feb 18, 2021 • edited

Codecov Report

ptvrajsk left a comment

Choose a reason for hiding this comment

ptvrajsk Feb 18, 2021

Choose a reason for hiding this comment

seanlowjk Feb 18, 2021

Choose a reason for hiding this comment

ptvrajsk Feb 18, 2021

Choose a reason for hiding this comment

seanlowjk Feb 18, 2021

Choose a reason for hiding this comment

anubh-v left a comment

Choose a reason for hiding this comment

seanlowjk commented Feb 18, 2021

seanlowjk commented Feb 19, 2021

seanlowjk commented Feb 20, 2021

anubh-v commented Feb 22, 2021 • edited

seanlowjk commented Feb 22, 2021

anubh-v left a comment

Choose a reason for hiding this comment

anubh-v commented Feb 24, 2021

seanlowjk commented Feb 24, 2021

seanlowjk commented Feb 11, 2021 •

edited

codecov-io commented Feb 18, 2021 •

edited

anubh-v commented Feb 22, 2021 •

edited