Skip to content

Expose Replace* methods for 2.x - #2435

Merged
Amir-61 merged 1 commit into
2.xfrom
expose_endpoints_2.x
Aug 15, 2016
Merged

Expose Replace* methods for 2.x#2435
Amir-61 merged 1 commit into
2.xfrom
expose_endpoints_2.x

Conversation

@Amir-61

@Amir-61 Amir-61 commented Jun 14, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Patch description:

*Re-mapping updateAttributes endpoint to use
PATCH and PUT(configurable) verb
*Exposing replaceById and replaceOrCreate via
POST and PUT(configurable) verb

What are the differences between this ptach for 2.x Vs. patch for 3.x:

  1. Default value for replaceOnPUT would be set to false; please see here

  2. In this patch for 2.x, we preserve upsert and set pachOrCreate as an alias only and change tests accordingly.

  3. In this patch for 2.x, we preserve updateAttributes and set pachAttributes as an alias only and change tests accordingly.

  4. Question: JSdoc for PersistedModel Methods are generated based on lib\persisted-models.js, so once this patch gets landed I believe docs for replaceOrCreate, replaceAttributes and replaceById gets added to our API docs, are we fine with that now?

PS: I squash my commits as one single commit after getting an LGTM. Thanks!

Connect to https://github.com/strongloop-internal/scrum-loopback/issues/917

/to: @bajtos

@Amir-61 Amir-61 changed the title Expose Replace* methods Expose Replace* methods for 2.x Jun 15, 2016
@Amir-61 Amir-61 removed their assignment Jun 15, 2016
@bajtos

bajtos commented Jun 20, 2016

Copy link
Copy Markdown
Member

Question: JSdoc for PersistedModel Methods are generated based on lib\persisted-models.js, so once this patch gets landed I believe docs for replaceOrCreate, replaceAttributes and replaceById gets added to our API docs, are we fine with that now?

I don't see why it would be a bad thing to expose API docs for the two new methods, what am I missing?

@bajtos bajtos assigned Amir-61 and unassigned bajtos Jun 20, 2016
bajtos
bajtos reviewed Jun 20, 2016
View reviewed changes
Comment thread lib/persisted-model.js
// This is just for LB 2.x
options.replaceOnPUT = options.replaceOnPUT === true;

function setRemoting(scope, name, options) {

@bajtos bajtos Jun 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be set to false by default, shouldn't it?

@Amir-61 Amir-61 Jun 20, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos:

I believe options.replaceOnPUT = options.replaceOnPUT === true; by default sets to false, isn't it? Lets see:
If options.replaceOnPUT is undefined undefined === true => false
If options.replaceOnPUT is false false === true => false
If options.replaceOnPUT is true true === true => true

Am I missing something?

@bajtos bajtos Jun 22, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, this is my fault, I overlooked === and thought it's =.

@Amir-61 Amir-61 assigned bajtos and 0candy and unassigned bajtos, Amir-61 and 0candy Jun 21, 2016
@bajtos

bajtos commented Jun 22, 2016

Copy link
Copy Markdown
Member

LGTM

@bajtos bajtos assigned Amir-61 and unassigned bajtos Jun 22, 2016
@Amir-61

Amir-61 commented Jun 23, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

@bajtos Lets please not merge this PR for now. I had a talk with @qpresley and we decided to have a discussion in F2F meeting about other IBM connectors (cloudant, informix. dashDB, DB2, DB2z, mqlight,...) support for replace methods before landing this PR. Mentioned connectors need to be tested as well; they are not on CI.

Thanks!

@Amir-61 Amir-61 changed the title Expose Replace* methods for 2.x DONOT MERGE-Expose Replace* methods for 2.x Jun 23, 2016
@Amir-61 Amir-61 changed the title DONOT MERGE-Expose Replace* methods for 2.x DO NOT MERGE-Expose Replace* methods for 2.x Jun 23, 2016
@Amir-61 Amir-61 mentioned this pull request Jul 28, 2016
Merged
@Amir-61
Amir-61 force-pushed the expose_endpoints_2.x branch 2 times, most recently from 0dd7d54 to 1bf08db Compare August 10, 2016 02:00
@Amir-61

Amir-61 commented Aug 12, 2016

Copy link
Copy Markdown
Member Author

@laven26 @ssh24,

So I'm about to land this PR which exposes our replaceById, replaceAttributes and replaceOrCreate; if you are done with the QA for these methods for supported IBM DBs, could you please confirm it here, so I can land it this PR.

Thanks!

@ssh24

ssh24 commented Aug 12, 2016

Copy link
Copy Markdown
Contributor

mssql, mysql, oracl, mongodb and postgres all work as expected

@ghost

ghost commented Aug 12, 2016

Copy link
Copy Markdown

DB2, DashDB and Cloudant all work as expected

@Amir-61
Expose Replace* methods
e562137 
*Re-mapping `updateAttributes` endpoint to use
`PATCH` and `PUT`(configurable) verb
*Exposing `replaceById` and `replaceOrCreate` via
`POST` and `PUT`(configurable) verb
@Amir-61
Amir-61 force-pushed the expose_endpoints_2.x branch from 1bf08db to e562137 Compare August 15, 2016 16:17
@Amir-61 Amir-61 changed the title DO NOT MERGE-Expose Replace* methods for 2.x Expose Replace* methods for 2.x Aug 15, 2016
@Amir-61
Amir-61 merged commit 6c9df36 into 2.x Aug 15, 2016
@Amir-61
Amir-61 deleted the expose_endpoints_2.x branch August 15, 2016 19:05
@Amir-61 Amir-61 removed the #review label Aug 15, 2016
@bajtos bajtos mentioned this pull request Aug 17, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Amir-61 Amir-61

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Amir-61 @bajtos @ssh24 @0candy