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

[FEATURE] Get the number of subscribers of list and added tests #116

Merged
merged 7 commits into from May 31, 2019

Conversation

Projects
None yet
3 participants
@xh3n1
Copy link
Member

commented Jan 27, 2019

Fix #115

@xh3n1 xh3n1 force-pushed the subscribers-number branch from 8fb00c7 to 79781fa Feb 14, 2019

get the number of subscribers of list, Fix #115
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>

@xh3n1 xh3n1 force-pushed the subscribers-number branch from 79781fa to 63de167 Feb 14, 2019

Add empty line before param annotation
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@xh3n1

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@oliverklee Thank you for the feedback. I think that I have made the requested changes and also added a test for a list without subscribers.

Show resolved Hide resolved src/Controller/ListController.php Outdated
Show resolved Hide resolved src/Controller/ListController.php
Show resolved Hide resolved tests/Integration/Controller/AbstractControllerTest.php Outdated
Show resolved Hide resolved tests/Integration/Controller/ListControllerTest.php Outdated
Show resolved Hide resolved tests/Integration/Controller/ListControllerTest.php Outdated

xh3n1 added some commits Mar 19, 2019

Make the intent clearer with the description and the name
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Add the comma after the last array element
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@oliverklee

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Hi @xh3n1, could you please fix the build failures? Then I'll take a look at the updated PR. Thanks!

@xh3n1

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

@oliverklee yes, thanks!

@oliverklee

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

@xh3n1 Do you run the tests locally before you push to GitHub?

Change function name to Countsubscribers, fix the tests
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>

@xh3n1 xh3n1 force-pushed the subscribers-number branch from e61c0f4 to b5985f6 Apr 3, 2019

@xh3n1

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@oliverklee couldn't run them before because of my environment and it was easier this way.
Now it's ready for review. Thanks.

@xh3n1 xh3n1 changed the title Get the number of subscribers of list and added tests [FEATURE] Get the number of subscribers of list and added tests Apr 3, 2019

@xh3n1

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@oliverklee any further changes I should do before this is ready to go?

@oliverklee

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

couldn't run them before because of my environment and it was easier this way.

You really need to have a local setup where you can run the unit tests, functional tests and style checks. If you need TravisCI to run the tests for you, the feedback loop is far too long for you to work productively and have flow while developing.

@oliverklee

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

@xh3n1 Have you seen this comment?

Will this actually retrieve all subscribers? Or will this only get the number of subscribers? For performance reasons, I'd prefer the latter.

Show resolved Hide resolved src/Controller/ListController.php Outdated
Show resolved Hide resolved tests/Integration/Controller/ListControllerTest.php Outdated
@xh3n1

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@oliverklee It gets only the number of subscribers, and I fixed my local setup to run my tests now.

Change the function name to getCountSubscribersAction
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
Change the name to SubscribersCount
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@oliverklee
Copy link
Collaborator

left a comment

Looks good. Thanks!

@oliverklee oliverklee added this to the 4.0.0 phase 3 milestone May 31, 2019

@oliverklee oliverklee merged commit cb7833a into master May 31, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@oliverklee oliverklee deleted the subscribers-number branch May 31, 2019

@oliverklee

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

Oh, it looks like this has broken the build. @xh3n1 Would you be willing to look into this?

@xh3n1

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

@oliverklee yes, I know why. Will create a PR later today.

xh3n1 added a commit that referenced this pull request Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.