Repository: ViacomInc/data-point
Pull request #317 (2018-12-20 18:33:12)

feat(data-point): new logic to resolve Request.url
<!--
Thanks for your interest in the project. We appreciate bugs filed and PRs submitted!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!

BREAKING CHANGES:

If your PR includes a breaking change, please submit it with a codemod under
data-point-codemods that will help users upgrade their codebase.

Breaking changes without a codemod will not be accepted unless a codemod is not
viable or does not apply to the specific situation.
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**:

will now use acc.value if request.url is empty and acc.value is a non-empty string

<!-- Why are these changes necessary? -->
**Why**:

#313

<!-- How were these changes implemented? -->
**How**:

by adding logic to check if request.url is empty and acc.value is a non-empty string, then use acc.value

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Has Breaking changes N/A
- [X] Documentation
- [X] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added username to **all-contributors** list N/A

<!-- feel free to add additional comments -->


codecov[bot]: # [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=h1) Report
> Merging [#317](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=desc) into [master](https://codecov.io/gh/ViacomInc/data-point/commit/bd0c8d418f2d8c09978ed3988bd8756f15f6f778?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/ViacomInc/data-point/pull/317/graphs/tree.svg?width=650&token=LYxItnrx4i&height=150&src=pr)](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           master   #317   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         137    137           
  Lines        2003   2004    +1     
  Branches      184    186    +2     
=====================================
+ Hits         2003   2004    +1
```


| [Impacted Files](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...a-point/lib/entity-types/entity-request/resolve.js](https://codecov.io/gh/ViacomInc/data-point/pull/317/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9saWIvZW50aXR5LXR5cGVzL2VudGl0eS1yZXF1ZXN0L3Jlc29sdmUuanM=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=footer). Last update [bd0c8d4...75c3c41](https://codecov.io/gh/ViacomInc/data-point/pull/317?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/packages/data-point/README.md b/packages/data-point/README.md
index 70ed157..e7070a8 100644
--- a/packages/data-point/README.md
+++ b/packages/data-point/README.md
@@ -2133,7 +2133,9 @@ dataPoint.addEntities({
 
 #### <a name="request-url">Request.url</a>
 
-Sets the url to be requested.
+Sets the url to be requested. 
+
+**NOTE:** When `Request.url` is not defined it will use the current `Accumulator.value` (if the value is of type `string`) as the Request's url.
 
 <details>
   <summary>`Request.url` Example</summary>
diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.js b/packages/data-point/lib/entity-types/entity-request/resolve.js
index 63ee3ad..cf6fe20 100644
--- a/packages/data-point/lib/entity-types/entity-request/resolve.js
+++ b/packages/data-point/lib/entity-types/entity-request/resolve.js
@@ -41,10 +41,10 @@ module.exports.getRequestOptions = getRequestOptions
  * @return {Promise<Accumulator>}
  */
 function resolveOptions (accumulator, resolveReducer) {
-  accumulator = resolveUrl(accumulator)
+  const url = resolveUrl(accumulator)
   const specOptions = accumulator.reducer.spec.options
   return resolveReducer(accumulator, specOptions).then(acc => {
-    const options = getRequestOptions(acc.url, acc.value)
+    const options = getRequestOptions(url, acc.value)
     return utils.assign(accumulator, { options })
   })
 }
@@ -74,9 +74,18 @@ module.exports.resolveUrlInjections = resolveUrlInjections
  * @return {Accumulator}
  */
 function resolveUrl (acc) {
-  const entity = acc.reducer.spec
-  const url = resolveUrlInjections(entity.url, acc)
-  return utils.assign(acc, { url })
+  let urlToResolve = acc.reducer.spec.url
+
+  // use acc.value when Request.url is not set
+  if (!urlToResolve && typeof acc.value === 'string' && acc.value) {
+    urlToResolve = acc.value
+  }
+
+  // prevent from executing resolveUrlInjections when
+  // urlToResolve is empty string
+  return urlToResolve && typeof urlToResolve === 'string'
+    ? resolveUrlInjections(urlToResolve, acc)
+    : undefined
 }
 
 module.exports.resolveUrl = resolveUrl


Test patch:
diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js
index 43a5f88..22fd8ed 100644
--- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js
+++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js
@@ -77,19 +77,67 @@ describe('resolveUrlInjections', () => {
 })
 
 describe('resolveUrl', () => {
-  test('It should set acc.url value', () => {
-    const acc = Resolve.resolveUrl({
+  test('It should get spec.url', () => {
+    const result = Resolve.resolveUrl({
       reducer: {
         spec: {
           url: 'http://foo'
         }
       }
     })
-    expect(acc).toHaveProperty('url', 'http://foo')
+    expect(result).toBe('http://foo')
+  })
+
+  it('should fallback on acc.value when spec.url is not present', () => {
+    const result = Resolve.resolveUrl({
+      value: 'http://fallback.com',
+      reducer: {
+        spec: {
+          url: undefined
+        }
+      }
+    })
+    expect(result).toBe('http://fallback.com')
+  })
+
+  it('should not fallback on acc.value if acc.value is empty', () => {
+    const result = Resolve.resolveUrl({
+      value: '',
+      reducer: {
+        spec: {
+          url: undefined
+        }
+      }
+    })
+    expect(result).toBe(undefined)
+  })
+
+  it('should return undefined if Request.url is empty and acc.value empty but both are strings', () => {
+    const result = Resolve.resolveUrl({
+      value: '',
+      reducer: {
+        spec: {
+          url: ''
+        }
+      }
+    })
+    expect(result).toBe(undefined)
+  })
+
+  it('should return undefined when neither spec.url is present or value is string', () => {
+    const result = Resolve.resolveUrl({
+      value: {}, // not a string
+      reducer: {
+        spec: {
+          url: undefined
+        }
+      }
+    })
+    expect(result).toBe(undefined)
   })
 
   test('It should do injections', () => {
-    const acc = Resolve.resolveUrl({
+    const result = Resolve.resolveUrl({
       value: 'bar',
       reducer: {
         spec: {
@@ -97,7 +145,7 @@ describe('resolveUrl', () => {
         }
       }
     })
-    expect(acc).toHaveProperty('url', 'http://foo/bar')
+    expect(result).toBe('http://foo/bar')
   })
 })
 
@@ -329,52 +377,46 @@ describe('resolve', () => {
     })
   })
 
-  test("interpolate data that's returned from the value lifecycle method", () => {
+  it('should use acc.value as url when request.url is not defined', () => {
     nock('http://remote.test')
-      .get('/source5')
+      .get('/source1')
       .reply(200, {
         ok: true
       })
 
-    return transform('request:a1.4', {
-      source: 'source5'
-    }).then(result => {
-      expect(result.value).toEqual({
-        ok: true
-      })
-    })
+    return transform('request:a3', 'http://remote.test/source1').then(
+      result => {
+        expect(result.value).toEqual({
+          ok: true
+        })
+      }
+    )
   })
 
-  test('url injections', () => {
+  test("interpolate data that's returned from the value lifecycle method", () => {
     nock('http://remote.test')
-      .get('/source1')
+      .get('/source5')
       .reply(200, {
         ok: true
       })
 
-    return transform('request:a1.0', {}).then(result => {
+    return transform('request:a1.4', {
+      source: 'source5'
+    }).then(result => {
       expect(result.value).toEqual({
         ok: true
       })
     })
   })
 
-  test('use contextPath', () => {
+  test('url injections', () => {
     nock('http://remote.test')
       .get('/source1')
       .reply(200, {
         ok: true
       })
 
-    return transform(
-      'request:a3',
-      {},
-      {
-        initialValue: {
-          itemPath: '/source1'
-        }
-      }
-    ).then(result => {
+    return transform('request:a1.0', {}).then(result => {
       expect(result.value).toEqual({
         ok: true
       })
diff --git a/packages/data-point/lib/entity-types/integrations.test.js b/packages/data-point/lib/entity-types/integrations.test.js
index 5faaac7..716ea3b 100644
--- a/packages/data-point/lib/entity-types/integrations.test.js
+++ b/packages/data-point/lib/entity-types/integrations.test.js
@@ -45,6 +45,20 @@ test('Entry#resolve - branch/leaf nesting', () => {
     })
 })
 
+test('Request should use resolved value as url, when url is missing', () => {
+  const expected = {
+    ok: true
+  }
+
+  nock('http://remote.test')
+    .get('/source1')
+    .reply(200, expected)
+
+  return dataPoint.transform('request:a3.1', {}).then(result => {
+    expect(result.value).toEqual(expected)
+  })
+})
+
 test('Entry#resolve - resolve request', () => {
   const expected = {
     ok: true
diff --git a/packages/data-point/lib/reducer-types/reducer-list/resolve.test.js b/packages/data-point/lib/reducer-types/reducer-list/resolve.test.js
index 4d2c3f8..9845c78 100644
--- a/packages/data-point/lib/reducer-types/reducer-list/resolve.test.js
+++ b/packages/data-point/lib/reducer-types/reducer-list/resolve.test.js
@@ -127,9 +127,7 @@ describe('resolve#reducer.resolve - reducer request', () => {
   test('multiple models for reducer request', () => {
     nock('http://remote.test')
       .get('/source1')
-      .reply(200, {
-        itemPath: '/source2'
-      })
+      .reply(200, 'http://remote.test/source2')
 
     nock('http://remote.test')
       .get('/source2')
diff --git a/packages/data-point/test/definitions/requests.js b/packages/data-point/test/definitions/requests.js
index 61983a7..ad594c8 100644
--- a/packages/data-point/test/definitions/requests.js
+++ b/packages/data-point/test/definitions/requests.js
@@ -40,17 +40,9 @@ module.exports = {
     value: () => ({ source: 'source5' }),
     url: 'http://remote.test/{value.source}'
   },
-  'request:a2': {
-    url: 'http://remote.test',
-    options: {
-      url: options => options.url + '/source1'
-    }
-  },
-  'request:a3': {
-    url: 'http://remote.test',
-    options: {
-      url: (options, acc) => acc.url + acc.initialValue.itemPath
-    }
+  'request:a3': {},
+  'request:a3.1': {
+    value: () => 'http://remote.test/source1'
   },
   'request:a3.2': {
     url: 'http://remote.test{locals.itemPath}'