Allow Notifier component to limit amount of notifications to return. #8
Conversation
README has instructions for method `allNotificationList()`, but component does not have this method. Replacing instruction with `getNotifications()` instead.
Change `getNotifications()` method to accept a 3rd parementer for $limit and make code changes to pull from DB
|
Hi @akkaweb, First of all, thank you for the PR! However, I have some suggestions for you.
If you have questions, feel free to chat me! Looking forward to your changes! Good luck! |
|
My apologies about that. I mostly develop for Drupal and my IDE is setup to work with their coding standards. I will fix and resubmit. |
Comply with CakePHP coding standards
Added 2 test cases for the newly added $limit parameter
|
I updated the code to comply with CakePHP standards. I will watch Travis to see if it allows it to pass now. Test cases have been added now. |
| @@ -96,11 +96,20 @@ Of course you want to get a list of notifications per user. Here are some exampl | |||
| // getting a list of all notifications of the user with id 2 | |||
| $this->Notifier->getNotifications(2); | |||
|
|
|||
| // getting a list of all unread notifications | |||
| $this->Notifier->allNotificationList(2, true); | |||
| // getting a list of all unread notifications for user with ID 2 | |||
There was a problem hiding this comment.
I documented with 'id', here you did it with 'ID' and below you typed 'Id'. What will it be? I suggest 'id'.
|
@akkaweb Thank you! Good work! I added some comments you should look at. Also the code-coverage gave one error: https://travis-ci.org/cakemanager/cakephp-notifier/jobs/118129153#L278. If you can fix these, it will be okay! As I see, you added a tirth parameter the the method, which is good enough. But I was tinking, to be prepared of the future, we could make the tirth parameter an array, with the option |
Change 3rd parameter to an array which allows the possibility for future expansion. Currently it only supports 'limit' - $options['limit']
Making changes to documentation to include $options array as 3rd parameter. Currently it only supports 'limit' - $options['limit']
Updating test cases to accept array as 3rd parameter with 'limit' as only key support at the moment
|
I totally agree about the 3rd parameter being an array. Thanks for catching that! I put that change into place, changed test cases to reflect that update and I also updated code-coverage to include the 3rd option. Hopefully all the tests will now pass. If any issues or if you would like something changed let me know. Thanks |
Travis check failed to whitespace at the end of line and a non-existent space after If. Should pass now!
|
Sorry for leaving this behind @akkaweb. Lets finish this :) I see you did 9 commits. I would like to see the commits squased (read more at http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html and http://couscous.io/contributing.html) Greetz, Bob |
|
@bobmulder Sure! I will give it a go soon! Thanks! |
| @@ -90,17 +90,29 @@ be defined here. | |||
| ### Lists | |||
| Of course you want to get a list of notifications per user. Here are some examples: | |||
|
|
|||
| // getting a list of all notifications of the current logged in user | |||
| // getting a list of all notifications of the current Logged In user | |||
There was a problem hiding this comment.
I dont think that change was valid :)
Changes to the method
getNotifications()to accept a 3rd paremeter$limit = nulland make the necessary code changes to accept limit. README.md also changed to reflect Component changes.