Skip to content

adding upsert method for folders#3

Open
ekump wants to merge 1 commit into
Skycatch:masterfrom
shake-apps:ek-upsert-folder
Open

adding upsert method for folders#3
ekump wants to merge 1 commit into
Skycatch:masterfrom
shake-apps:ek-upsert-folder

Conversation

@ekump

@ekump ekump commented Mar 9, 2015

Copy link
Copy Markdown

add a method that will attempt to create a folder. If there is already a folder in that location with that name return that folder. Otherwise, create a new one.

@lyaunzbe

lyaunzbe commented Mar 9, 2015

Copy link
Copy Markdown
Contributor

Looks good, will merge soon. I also think I will just rename this method to 'create' and replace the current 'create' method. Thanks for your contribution.

@srt32

srt32 commented May 21, 2015

Copy link
Copy Markdown
Contributor

Ping @lyaunzbe, curious what your thoughts / plans are for this idea of upserting.

@srt32

srt32 commented Aug 28, 2015

Copy link
Copy Markdown
Contributor

@ekump do you know the status of this PR? I'm looking to implement something similar on a project and I'd enjoy not doing it myself ;)

@lyaunzbe

Copy link
Copy Markdown
Contributor

@srt32 I will gladly merge this in (it looks good to me),but do you mind just testing @ekump's branch to confirm that it works and leaving a comment here? I'd do it myself but currently don't have the time.

@srt32

srt32 commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

@lyaunzbe I can test early this week. Would you be open to my taking a stab at a basic test suite as well?

@lyaunzbe

lyaunzbe commented Sep 6, 2015

Copy link
Copy Markdown
Contributor

Of course @srt32 ! Your contributions would be much appreciated.

@srt32

srt32 commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

Sorry about the delay here. I got swamped at work. I plan to tackle this manual test over the next week.

srt32 pushed a commit to srt32/node-box that referenced this pull request Sep 28, 2015
* Adds a test for `files.upload` (without custom name).
* Adds `nock` as a `devDependency` for testing.

I'd enjoy any feedback the approach I took and would also be happy to
add coverage to some of the other functions if we're happy with the
general direction here.

Conversation started in
Skycatch#3 (comment)
srt32 pushed a commit to srt32/node-box that referenced this pull request Sep 28, 2015
* Adds a test for `files.upload` (without custom name).
* Adds `nock` as a `devDependency` for testing.

I'd enjoy any feedback on the approach I took and would also be happy to
add coverage to some of the other functions if we're happy with the
general direction here.

Conversation started in
Skycatch#3 (comment)
@srt32

srt32 commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

I added the start to a test suite in #12. If things look good there, I can add a similar kind of test for this new functionality (which I'm excited to use ;)).

srt32 pushed a commit to srt32/node-box that referenced this pull request Sep 28, 2015
* Adds a test for `files.upload` (without custom name).
* Adds `nock` as a `devDependency` for testing.

I'd enjoy any feedback on the approach I took and would also be happy to
add coverage to some of the other functions if we're happy with the
general direction here.

Conversation started in
Skycatch#3 (comment)
srt32 pushed a commit to srt32/node-box that referenced this pull request Sep 28, 2015
* Adds a test for `files.upload` (without custom name).
* Adds `nock` as a `devDependency` for testing.

I'd enjoy any feedback on the approach I took and would also be happy to
add coverage to some of the other functions if we're happy with the
general direction here.

Conversation started in
Skycatch#3 (comment)
@srt32

srt32 commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

I manually tested this functionality today and it works but the responses are not uniform. See below for more details.

Steps:

  • create a new folder in a known directory using upsert. The response was successful and looked like (some data changed for privacy reasons):
{ type: 'folder',
  id: '4770752700',
  sequence_id: '0',
  etag: '0',
  name: 'test_srt32',
  created_at: '2015-09-28T16:44:45-07:00',
  modified_at: '2015-09-28T16:44:45-07:00',
  description: '',
  size: 0,
  path_collection: { total_count: 2, entries: [ [Object], [Object] ] },
  created_by:
   { type: 'user',
     id: '248346000',
     name: 'user name',
     login: 'user@example.com' },
  modified_by:
   { type: 'user',
     id: '248346000',
     name: 'user name foo',
     login: 'user@example.com' },
  trashed_at: null,
  purged_at: null,
  content_created_at: '2015-09-28T16:44:45-07:00',
  content_modified_at: '2015-09-28T16:44:45-07:00',
  owned_by:
   { type: 'user',
     id: '237410600',
     name: 'user name',
     login: 'user@example.com' },
  shared_link: null,
  folder_upload_email: null,
  parent:
   { type: 'folder',
     id: '4535300400',
     sequence_id: '0',
     etag: '0',
     name: 'tmp' },
  item_status: 'active',
  item_collection:
   { total_count: 0,
     entries: [],
     offset: 0,
     limit: 100,
     order: [ [Object], [Object] ] } }
  • then, run the same command again. The response was successful and looked like:
{ type: 'folder',
  id: '4770752700',
  sequence_id: '0',
  etag: '0',
  name: 'test_srt32' }

The functionality worked but the response from the function will be different depending on which scenario we're in (as the response from Box is different). It seems like, ideally, we'd have the same response from this function regardless of what happens internally. Perhaps we wrap up the response in some way so that it's uniform?

@srt32

srt32 commented Oct 6, 2015

Copy link
Copy Markdown
Contributor

@ekump @lyaunzbe, curious about your thoughts regarding the return values of this function. Having the idempotent folder creation land would be great but I'm not sure if we want it as is currently.

@srt32

srt32 commented Nov 14, 2015

Copy link
Copy Markdown
Contributor

Ping @ekump @lyaunzbe, would either of you object to my taking over here and standardizing the response?

@ekump

ekump commented Nov 23, 2015

Copy link
Copy Markdown
Author

@srt32 No objections here.

@lyaunzbe

Copy link
Copy Markdown
Contributor

No objections, just submit the standardized request to this branch/pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants