Get, add, update and delete attachment modules. #235

COV-GIS · 2018-06-25T19:20:28Z

Here's the attachment modules for review. Tests and demo coming as soon as time allows.

coveralls · 2018-06-25T19:31:38Z

Coverage remained the same at 100.0% when pulling 8d25490 on COV-GIS:attachment-modules into 237f79e on Esri:master.

COV-GIS · 2018-06-26T15:08:04Z

packages/arcgis-rest-feature-service/src/addAttachment.ts

+ * @param params - Additional parameters to be sent via the request. See reference docs.
+ */
+export interface IAddAttachmentOptions extends IFeatureRequestOptions {
+  attachment: File;


@jgravois and @tomwayson I'm having an issue mocking up a File for testing. tslint wants a File b/c I've typed it here. But get a Reference Error when running tests. Do you have any thoughts on this. Here's the test...

it("should return objectId of the added attachment and a truthy success", done => { const requestOptions = { url: "https://services.arcgis.com/V6ZHFr6zdgNZuVG0/arcgis/rest/services/Landscape_Trees/FeatureServer/0", id: 42, attachment: new File(["foo"], "foo.txt", {type: "text/plain"}) as File, where: "1=1" } as IAddAttachmentOptions; fetchMock.once("*", addAttachmentResponse); addAttachment(requestOptions).then(response => { expect(fetchMock.called()).toBeTruthy(); const [url, options]: [string, RequestInit] = fetchMock.lastCall("*"); expect(url).toEqual(`${requestOptions.url}/${requestOptions.id}/addAttachment`); expect(options.body).toContain("where=1%3D1"); expect(options.method).toBe("POST"); expect(addAttachmentResponse.addAttachmentResult.objectId).toEqual(1001); expect(addAttachmentResponse.addAttachmentResult.success).toEqual(true); done(); }); });

@COV-GIS It is probally because File doesn't exist in Node so your getting a ReferenceError in the Node tests. Try running the following npm run test:chrome this will run only the browser tests. If that passes then run npm run test:node if that fails then you probably need to detect if File exists and if it doesn't do https://stackoverflow.com/questions/44021538/how-to-send-a-file-in-request-node-fetch-or-nodejs to create a different object to use for the attachment.

Thank you @patrickarlt.

COV-GIS · 2018-06-26T17:34:48Z

Couple notes on the tests...

I added and exported attachmentFile() in feature.ts but the code coverage goes down, so moved to features.test.ts.
Custom params are not being added to addAttachment() or updateAttachment(). expect(options.body).toContain("where=1%3D1"); fails when where: "1=1" is added to request options and I can't see any reason why.

jgravois

very strong pull request. we really appreciate you going to the trouble of contributing this work.

jgravois · 2018-07-03T16:48:44Z

packages/arcgis-rest-feature-service/src/addAttachment.ts

@@ -0,0 +1,66 @@
+/* Copyright (c) 2017 Environmental Systems Research Institute, Inc.


jgravois · 2018-07-03T16:54:00Z

packages/arcgis-rest-feature-service/src/addAttachment.ts

+  };
+
+  // `attachment` and any additional parameters --> params: {}
+  appendCustomParams(requestOptions, options);


since attachment is the only custom parameter this method supports, we're better off appending it manually and skip using the utility method.

the reason i used appendCustomParams()in the crud operations for feature service is because the interfaces they extend (ISharedQueryParams and IEditFeaturesParams) include quite a few optional parameters that are a little too tedious to test for and append manually.

at this point i'm actually more fond of the approach you've taken here, primarily because they make calling appendCustomParams() superfluous.

Hey @jgravois . attachment is not the only param for the operation. But they are few. Per your comment below about being explicit, why not a just extend IRequestOptions for each of the attachment methods with the REST API's params. I suppose I could have an interface with params like gdbVersion, but I like the simplicity of not extending several interfaces with these straight forward server operations. Honestly, I find the reference hard to follow when it's 3-4 interfaces deep. Based on this project's description it should be the thinnest of wrappers for the REST API without piling on. Just my two cents.

Let me know what you think either way, and I'll get it fixed up.

export interface IAddAttachmentOptions extends IRequestOptions { url: string; featureId: number; attachment: File; gdbVersion?: string; returnEditMoment?: boolean; uploadId?: number; } export function addAttachment( requestOptions: IAddAttachmentOptions ): Promise<IAddAttachmentResponse> { const options: IAddAttachmentOptions = { params: {}, ...requestOptions }; // `attachment` and any additional parameters --> params: {} options.params.attachment = requestOptions.attachment; options.params.gdbVersion = requestOptions.gdbVersion || null; options.params.returnEditMoment = requestOptions.returnEditMoment === true ? true : false; options.params.uploadId = requestOptions.uploadId || null; // force POST options.httpMethod = "POST"; return request( `${requestOptions.url}/${requestOptions.featureId}/addAttachment`, options ); }

Based on this project's description it should be the thinnest of wrappers for the REST API without piling on

i agree 💯%.
i was suggesting this:

export interface IAddAttachmentOptions extends IRequestOptions { url: string; featureId: number; attachment: File; } export function addAttachment( requestOptions: IAddAttachmentOptions ): Promise<IAddAttachmentResponse> { const options: IAddAttachmentOptions = { params: {}, ...requestOptions }; // pass through `attachment` as a request parameter manually options.params.attachment = requestOptions.attachment; // options.httpMethod = "POST"; no need to enforce the default return request( `${options.url}/${options.featureId}/addAttachment`, options ); }

then folks can still exploit the more esoteric parameters like this:

addAttachment({ url, featureId, attachment, params: { returnEditMoment: true } })

make sense?

Does the sun rise in the east?

I'll get it all fixed up.

jgravois · 2018-07-03T16:56:05Z

packages/arcgis-rest-feature-service/src/addAttachment.ts

+ * addAttachment({
+ *   url: "https://sampleserver6.arcgisonline.com/arcgis/rest/services/ServiceRequest/FeatureServer/0",
+ *   id: 8484,
+ *   attachment: myFileInput.files[0]


i don't know if there is a terse enough way to do this in a code snippet, but it'd be useful to show in more detail what the input file actually has to look like.

if necessary, we could also just create a demo app.

Yes. I've started an attachment demo.

jgravois · 2018-07-03T16:57:24Z

packages/arcgis-rest-feature-service/src/addAttachment.ts

+ * @param url - Feature service url.
+ * @param id - Unique identifier of feature to add related attachment.
+ * @param attachment - File to be attached.
+ * @param params - Additional parameters to be sent via the request. See reference docs.


no need to doc params. this is inherited from request.

jgravois · 2018-07-03T17:00:19Z

packages/arcgis-rest-feature-service/src/deleteAttachments.ts

+ * Request options to for deleting related attachments of a feature by id. See [Delete Attachments](https://developers.arcgis.com/rest/services-reference/delete-attachments.htm) for more information.
+ *
+ * @param url - Feature service url.
+ * @param id - Unique identifier of feature to delete related attachment(s).


another nitpick, but i think it makes more sense to have the generic id refer to the attachments than the feature in this method.

~~id~~ > featureId
~~attachmentIds~~ > ids

or we could just be completely explicit.

featureId
attachmentIds

jgravois · 2018-07-03T17:03:35Z

packages/arcgis-rest-feature-service/src/addAttachment.ts

+  // force POST
+  options.httpMethod = "POST";
+
+  return request(


there's no error in your logic, but since you already mixed in, it'd be more consistent to use options exclusively and stop relying on requestOptions altogether.

What does that pattern look like generally?

i just meant this:

const options: IAnyAttachmentOptions = { params: {}, ...requestOptions }; return request( `${options.url}/${options.id}/any`, options // instead of requestOptions );

jgravois · 2018-07-03T17:06:31Z

packages/arcgis-rest-feature-service/src/getAttachments.ts

+ * @param params - Additional parameters to be sent via the request. See reference docs.
+ */
+// Use IFeatureRequestOptions directly.
+// export interface IGetAttachmentsOptions extends IFeatureRequestOptions {}


safe to remove.

jgravois · 2018-07-03T17:07:43Z

packages/arcgis-rest-feature-service/test/features.test.ts

 } from "./mocks/feature";

+function attachmentFile() {


jgravois · 2018-07-03T17:08:34Z

packages/arcgis-rest-feature-service/test/features.test.ts

+  it("should return an array of attachmentInfos for a feature by id", done => {
+    const requestOptions = {
+      url:
+        "https://services.arcgis.com/V6ZHFr6zdgNZuVG0/arcgis/rest/services/Landscape_Trees/FeatureServer/0",


it'd be good to use a visibly fake url here.

jgravois · 2018-07-03T17:09:49Z

packages/arcgis-rest-feature-service/test/features.test.ts

+      ).toEqual(true);
+      done();
+    });
+  });


can you add one more test to trap for the scenario in which a developer tries to add an attachment to a feature service that doesn't support the capability please?

Missed this. Just did.

jgravois · 2018-07-03T17:44:06Z

packages/arcgis-rest-feature-service/test/features.test.ts

+      url:
+        "https://services.arcgis.com/V6ZHFr6zdgNZuVG0/arcgis/rest/services/Landscape_Trees/FeatureServer/0",
+      id: 42,
+      where: "1=1"


unless i'm missing something, the underlying operation for getAttachments() doesn't support a where parameter.

It does not. But was just testing whether addition params made it through. The crud methods don't either but where is passed and tested for.

COV-GIS · 2018-07-09T20:53:16Z

@jgravois I'll get to this hopefully soon.

COV-GIS · 2018-07-18T00:57:56Z

packages/arcgis-rest-feature-service/test/features.test.ts

+        `${requestOptions.url}/${requestOptions.featureId}/addAttachment`
+      );
+      expect(options.method).toBe("POST");
+      // expect(options.body).toContain("returnEditMoment=true");


I'm not sure what's happening in the tests, but any additional param passed with add and update fail on account of the body supposedly not containing the param. However I think it has to do with passing the file and test suit. The messages are gibberish to me. I successfully pass additional params in the demo.

this is because the request parameters are passed as FormData. the approach below works in browsers, but throws an error in Node.js

ref: form-data/form-data#124

expect(options.body instanceof FormData).toBeTruthy(); const params = options.body as FormData; expect(params.get("returnEditMoment")).toEqual("true");

all in all, this PR looks great. i haven't checked out your demo yet, but i've pushed a commit of my own to tidy up a few more things i've noticed so far.

COV-GIS · 2018-07-18T01:04:15Z

I think this covers it @jgravois. Let me know if there's any other changes and if not I'll get the single commit pushed.

I did come across a related and interesting item of note.

AGS will return a valid attachmentInfos response for a feature which does not exist.

* move attachment tests into their own file * fix one code snippet * stop setting POST manually (its the default) * dont cast an options object internally when its not necessary * switch done() with fail() when the block shouldnt be entered

review has been addressed, but more changes may be needed.

COV-GIS · 2018-07-19T14:09:37Z

That all looks good @jgravois. I added polyfills to demo. And hopefully we can get this wrapped up.

* upstream/master: missed one chore(groups): remove duplicate IGroup interface (and extend IItem) chore(all): get pkg.versions back in sync v1.5.1 fix(users): users defaults to https://www.arcgis.com instead of http:// v1.5.0 address feedback feat(:unlock:): add support for an OAuth flow that triggers social media login automatically fix(enterprise): fetch fresh token manually when u/pw are provided v1.4.2 fix(:bathtub:): ensure add/update/deleteFeatures dont pass extraneous parameters Fix peer dependency name

* confirmed demo app works (even prior to publishing package) * switched to clearly fake feature service urls in tests * fixed my own typo

…-modules * cov/attachment-modules: Add polyfills to demo.

jgravois

the demo doesn't need a disclaimer, we just needed to get the actual pkg.version in sync in order to convince lerna to symlink it instead of trying to use the npm registry.

i synced with master to confirm this and made a few more small tweaks to the tests and demo.

🎸 thanks again for this awesome contribution! 🚀

…ttachments ISSUES CLOSED: #226

COV-GIS · 2018-07-19T18:05:44Z

Thanks @jgravois. Can't wait to put this to work!

I can only foresee adding related records query. I've learned a lot...so hope to hit that one out of the park with a lower commit and comment count.

jgravois · 2018-07-19T18:44:08Z

a lower commit and comment count.

dialogue and revision means the system is working, not failing 😄.

i'll release as soon as i have a chance to plow through a few other pending PRs. i doubt it'll take more than a week.

Get, add, update and delete attachment modules.

d64e5a2

COV-GIS commented Jun 26, 2018

View reviewed changes

COV-GIS added 2 commits June 26, 2018 10:20

Remove unneeded and wrong param

db7899b

Add tests for attachment modules.

356369d

jgravois previously requested changes Jul 3, 2018

View reviewed changes

jgravois reviewed Jul 3, 2018

View reviewed changes

Update attachment modules per review. And add a demo.

e765da9

COV-GIS commented Jul 18, 2018

View reviewed changes

COV-GIS and others added 2 commits July 18, 2018 08:53

Add attachment error test. Fix up demo a bit.

a1681a3

minor cleanup

d78e547

* move attachment tests into their own file * fix one code snippet * stop setting POST manually (its the default) * dont cast an options object internally when its not necessary * switch done() with fail() when the block shouldnt be entered

Add polyfills to demo.

7c38266

jgravois added 3 commits July 19, 2018 10:31

final cleanup

ab93f25

* confirmed demo app works (even prior to publishing package) * switched to clearly fake feature service urls in tests * fixed my own typo

Merge remote-tracking branch 'cov/attachment-modules' into attachment…

78267a0

…-modules * cov/attachment-modules: Add polyfills to demo.

jgravois approved these changes Jul 19, 2018

View reviewed changes

feat(:paperclip:): add support for interacting with feature service a…

8d25490

…ttachments ISSUES CLOSED: #226

jgravois merged commit daeec31 into Esri:master Jul 19, 2018

jgravois mentioned this pull request Jul 27, 2018

arcgis-rest-feature-service 1.5.1 : getAttachments is not a function #258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get, add, update and delete attachment modules. #235

Get, add, update and delete attachment modules. #235

COV-GIS commented Jun 25, 2018

coveralls commented Jun 25, 2018 •

edited

Loading

COV-GIS Jun 26, 2018

patrickarlt Jun 26, 2018

COV-GIS Jun 26, 2018

COV-GIS commented Jun 26, 2018 •

edited

Loading

jgravois left a comment

jgravois Jul 3, 2018

jgravois Jul 3, 2018

jgravois Jul 5, 2018 •

edited

Loading

COV-GIS Jul 17, 2018

jgravois Jul 17, 2018

COV-GIS Jul 17, 2018

jgravois Jul 3, 2018

COV-GIS Jul 17, 2018

jgravois Jul 3, 2018

jgravois Jul 3, 2018 •

edited

Loading

jgravois Jul 3, 2018 •

edited

Loading

COV-GIS Jul 17, 2018

jgravois Jul 17, 2018

jgravois Jul 3, 2018

jgravois Jul 3, 2018

jgravois Jul 3, 2018

jgravois Jul 3, 2018

COV-GIS Jul 18, 2018

jgravois Jul 3, 2018

COV-GIS Jul 17, 2018

COV-GIS commented Jul 9, 2018

COV-GIS Jul 18, 2018

jgravois Jul 19, 2018

COV-GIS commented Jul 18, 2018

COV-GIS commented Jul 19, 2018

jgravois left a comment

COV-GIS commented Jul 19, 2018

jgravois commented Jul 19, 2018

		@@ -0,0 +1,66 @@
		/* Copyright (c) 2017 Environmental Systems Research Institute, Inc.

Get, add, update and delete attachment modules. #235

Get, add, update and delete attachment modules. #235

Conversation

COV-GIS commented Jun 25, 2018

coveralls commented Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

COV-GIS commented Jun 26, 2018 • edited Loading

jgravois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgravois Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgravois Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

jgravois Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

COV-GIS commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

COV-GIS commented Jul 18, 2018

COV-GIS commented Jul 19, 2018

jgravois left a comment

Choose a reason for hiding this comment

COV-GIS commented Jul 19, 2018

jgravois commented Jul 19, 2018

coveralls commented Jun 25, 2018 •

edited

Loading

COV-GIS commented Jun 26, 2018 •

edited

Loading

jgravois Jul 5, 2018 •

edited

Loading

jgravois Jul 3, 2018 •

edited

Loading

jgravois Jul 3, 2018 •

edited

Loading