-
Notifications
You must be signed in to change notification settings - Fork 123
Added client-side cache to Api::getResolutions #131
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
Changes from all commits
d155c27
ff7efe2
19f33c7
eec6107
15c867e
ab67c8b
17c75f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,6 +149,114 @@ public function testFindVersionByName() | |
| ); | ||
| } | ||
|
|
||
| public function testGetResolutions() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these tests look almost the same. Proposing to create one universal test with a data provider, that would accept these parameters:
|
||
| { | ||
| $response = file_get_contents(__DIR__ . '/resources/api_resolution.json'); | ||
|
|
||
| $this->expectClientCall( | ||
| Api::REQUEST_GET, | ||
| '/rest/api/2/resolution', | ||
| array(), | ||
| $response | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| ); | ||
|
|
||
| $actual = $this->api->getResolutions(); | ||
|
|
||
| $response_decoded = json_decode($response, true); | ||
|
|
||
| $expected = array( | ||
| '1' => $response_decoded[0], | ||
| '10000' => $response_decoded[1], | ||
| ); | ||
| $this->assertEquals($expected, $actual); | ||
|
|
||
| // Second time we call the method the results should be cached and not trigger an API Request. | ||
| $this->client->sendRequest(Api::REQUEST_GET, '/rest/api/2/resolution', array(), self::ENDPOINT, $this->credential) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tried to remove caching layer in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that works |
||
| ->shouldNotBeCalled(); | ||
| $this->assertEquals($expected, $this->api->getResolutions(), 'Calling twice did not yield the same results'); | ||
| } | ||
|
|
||
| public function testGetFields() | ||
| { | ||
| $response = file_get_contents(__DIR__ . '/resources/api_field.json'); | ||
|
|
||
| $this->expectClientCall( | ||
| Api::REQUEST_GET, | ||
| '/rest/api/2/field', | ||
| array(), | ||
| $response | ||
| ); | ||
|
|
||
| $actual = $this->api->getFields(); | ||
|
|
||
| $response_decoded = json_decode($response, true); | ||
|
|
||
| $expected = array( | ||
| 'issuetype' => $response_decoded[0], | ||
| 'timespent' => $response_decoded[1], | ||
| ); | ||
| $this->assertEquals($expected, $actual); | ||
|
|
||
| // Second time we call the method the results should be cached and not trigger an API Request. | ||
| $this->client->sendRequest(Api::REQUEST_GET, '/rest/api/2/field', array(), self::ENDPOINT, $this->credential) | ||
| ->shouldNotBeCalled(); | ||
| $this->assertEquals($expected, $this->api->getFields(), 'Calling twice did not yield the same results'); | ||
| } | ||
|
|
||
| public function testGetStatuses() | ||
| { | ||
| $response = file_get_contents(__DIR__ . '/resources/api_status.json'); | ||
|
|
||
| $this->expectClientCall( | ||
| Api::REQUEST_GET, | ||
| '/rest/api/2/status', | ||
| array(), | ||
| $response | ||
| ); | ||
|
|
||
| $actual = $this->api->getStatuses(); | ||
|
|
||
| $response_decoded = json_decode($response, true); | ||
|
|
||
| $expected = array( | ||
| '1' => $response_decoded[0], | ||
| '3' => $response_decoded[1], | ||
| ); | ||
| $this->assertEquals($expected, $actual); | ||
|
|
||
| // Second time we call the method the results should be cached and not trigger an API Request. | ||
| $this->client->sendRequest(Api::REQUEST_GET, '/rest/api/2/status', array(), self::ENDPOINT, $this->credential) | ||
| ->shouldNotBeCalled(); | ||
| $this->assertEquals($expected, $this->api->getStatuses(), 'Calling twice did not yield the same results'); | ||
| } | ||
|
|
||
| public function testGetPriorities() | ||
| { | ||
| $response = file_get_contents(__DIR__ . '/resources/api_priority.json'); | ||
|
|
||
| $this->expectClientCall( | ||
| Api::REQUEST_GET, | ||
| '/rest/api/2/priority', | ||
| array(), | ||
| $response | ||
| ); | ||
|
|
||
| $actual = $this->api->getPriorities(); | ||
|
|
||
| $response_decoded = json_decode($response, true); | ||
|
|
||
| $expected = array( | ||
| '1' => $response_decoded[0], | ||
| '5' => $response_decoded[1], | ||
| ); | ||
| $this->assertEquals($expected, $actual); | ||
|
|
||
| // Second time we call the method the results should be cached and not trigger an API Request. | ||
| $this->client->sendRequest(Api::REQUEST_GET, '/rest/api/2/priority', array(), self::ENDPOINT, $this->credential) | ||
| ->shouldNotBeCalled(); | ||
| $this->assertEquals($expected, $this->api->getPriorities(), 'Calling twice did not yield the same results'); | ||
| } | ||
|
|
||
| /** | ||
| * Expects a particular client call. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| [ | ||
| { | ||
| "id": "issuetype", | ||
| "key": "issuetype", | ||
| "name": "Issue Type", | ||
| "custom": false, | ||
| "orderable": true, | ||
| "navigable": true, | ||
| "searchable": true, | ||
| "clauseNames": [ | ||
| "issuetype", | ||
| "type" | ||
| ], | ||
| "schema": { | ||
| "type": "issuetype", | ||
| "system": "issuetype" | ||
| } | ||
| }, | ||
| { | ||
| "id": "timespent", | ||
| "key": "timespent", | ||
| "name": "Time Spent", | ||
| "custom": false, | ||
| "orderable": false, | ||
| "navigable": true, | ||
| "searchable": false, | ||
| "clauseNames": [ | ||
| "timespent" | ||
| ], | ||
| "schema": { | ||
| "type": "number", | ||
| "system": "timespent" | ||
| } | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| [ | ||
| { | ||
| "self": "https://test.atlassian.net/rest/api/2/priority/1", | ||
| "statusColor": "#cc0000", | ||
| "description": "Blocks development and/or testing work, production could not run.", | ||
| "iconUrl": "https://test.atlassian.net/images/icons/priorities/blocker.svg", | ||
| "name": "Blocker", | ||
| "id": "1" | ||
| }, | ||
| { | ||
| "self": "https://test.atlassian.net/rest/api/2/priority/5", | ||
| "statusColor": "#003300", | ||
| "description": "Cosmetic problem like misspelt words or misaligned text.", | ||
| "iconUrl": "https://test.atlassian.net/images/icons/priorities/trivial.svg", | ||
| "name": "Trivial", | ||
| "id": "5" | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| [ | ||
| { | ||
| "self": "https://test.atlassian.net/rest/api/2/resolution/1", | ||
| "id": "1", | ||
| "description": "A fix for this issue is checked into the tree and tested.", | ||
| "name": "Fixed" | ||
| }, | ||
| { | ||
| "self": "https://test.atlassian.net/rest/api/2/resolution/10000", | ||
| "id": "10000", | ||
| "description": "This issue won't be actioned.", | ||
| "name": "Won't Do" | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| [ | ||
| { | ||
| "self": "https://test.atlassian.net/rest/api/2/status/1", | ||
| "description": "The issue is open and ready for the assignee to start work on it.", | ||
| "iconUrl": "https://test.atlassian.net/images/icons/statuses/open.png", | ||
| "name": "Open", | ||
| "id": "1", | ||
| "statusCategory": { | ||
| "self": "https://test.atlassian.net/rest/api/2/statuscategory/2", | ||
| "id": 2, | ||
| "key": "new", | ||
| "colorName": "blue-gray", | ||
| "name": "To Do" | ||
| } | ||
| }, | ||
| { | ||
| "self": "https://test.atlassian.net/rest/api/2/status/3", | ||
| "description": "This issue is being actively worked on at the moment by the assignee.", | ||
| "iconUrl": "https://test.atlassian.net/images/icons/statuses/inprogress.png", | ||
| "name": "In Progress", | ||
| "id": "3", | ||
| "statusCategory": { | ||
| "self": "https://test.atlassian.net/rest/api/2/statuscategory/4", | ||
| "id": 4, | ||
| "key": "indeterminate", | ||
| "colorName": "yellow", | ||
| "name": "In Progress" | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this approach in Symfony, but wouldn't
isset($this->fields)be more explicit (I've wrote about this in another comment)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to use strict type comparison instead of the more magicky methods like empty() and isset(). Matter of preference I guess. (empty("0") == true or false? :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Just use it consistently so we don't me different null checking styles in different places.
Yeah, the
emptyfunction is hell. I never use it.