Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support createOnlyInstance in model #3548

Merged
merged 5 commits into from
Aug 23, 2017
Merged

Support createOnlyInstance in model #3548

merged 5 commits into from
Aug 23, 2017

Conversation

rashmihunt
Copy link
Contributor

@rashmihunt rashmihunt commented Aug 5, 2017 •
edited
Loading

This is part of solution for #2924.

Other PRs involved in the solution of issue #2924

@bajtos Work In Progress PR. Please take a initial look.

Changes include

  • setting up createOnlyInstance in PersistentModel.js and model.js

TODO:

  • Add a test case

@rashmihunt rashmihunt requested a review from bajtos August 5, 2017 00:23
@rashmihunt rashmihunt changed the title WIP: createOnlyInstance support for updateOnly, forceId changes for https://github.com/strongloop/loopback/issues/2924 WIP: createOnlyInstance support for updateOnly, forceId changes for #2924 Aug 5, 2017
This was referenced Aug 5, 2017
Merged
Merged
bajtos
bajtos suggested changes Aug 7, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Nice! Please add unit-tests to cover both regular PersistedModel create and relation-based __create__${scopeName}.

lib/model.js Outdated
@@ -758,6 +758,14 @@ module.exports = function(registry) {
var pathName =
(scope.options && scope.options.http && scope.options.http.path) || scopeName;

var updateOnlyProps = this.getUpdateOnlyProperties();
var createOnly = false;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a more descriptive name please, e.g. hasUpdateOnlyProps.

Also please use const instead of var, see http://loopback.io/doc/en/contrib/style-guide.html#variable-declarations

const updateOnlyProps = this.getUpdateOnlyProperties();
const hasUpdateOnlyProps = updateOnlyProps && updateOnlyProps.length > 0;

Copy link
Member

Choose a reason for hiding this comment

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

This comment does not seem to be addressed yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos I have two environment 1) loopback application where I can test with the API designer end to end 2) my git hub branch where I can do run tests and checkins etc. I addressed above comments in the first env to make sure I test end-to-end through API Explorer with these code review comment changes and forgot to do same change in 2nd env/local git hub branch. Tests were passing since my previous code was logically correct. Anyway, done the above change now...

lib/persisted-model.js Outdated
@@ -627,6 +627,14 @@ module.exports = function(registry) {
var typeName = PersistedModel.modelName;
var options = PersistedModel.settings;

var updateOnlyProps = PersistedModel.getUpdateOnlyProperties();
var createOnly = false;
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@rashmihunt rashmihunt changed the title WIP: createOnlyInstance support for updateOnly, forceId changes for #2924 Support createOnlyInstance in model Aug 15, 2017
@rashmihunt
Copy link
Contributor Author

@bajtos PTAL

  • Incorporated code review comments
  • Added new tests

bajtos
bajtos suggested changes Aug 16, 2017
View reviewed changes
lib/model.js Outdated
@@ -758,6 +758,14 @@ module.exports = function(registry) {
var pathName =
(scope.options && scope.options.http && scope.options.http.path) || scopeName;

var updateOnlyProps = this.getUpdateOnlyProperties();
var createOnly = false;
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not seem to be addressed yet?

test/remoting.integration.js Outdated
@@ -316,6 +331,12 @@ function getFormattedMethodsExcludingRelations(methods) {
});
}

function getCreateMethod(methods) {
return methods.filter(function(m) {
Copy link
Member

Choose a reason for hiding this comment

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

I find it confusing that a function called getCreateMethod returns an array of instances. Please use methods.find instead of methods.filter - see https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/find

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos Done. PTAL

@rashmihunt
Copy link
Contributor Author

@slnode test please

bajtos
bajtos approved these changes Aug 21, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

test/remoting.integration.js
@@ -316,6 +331,12 @@ function getFormattedMethodsExcludingRelations(methods) {
});
}

function getCreateMethod(methods) {
return methods.find(function(m) {
return (m.name === 'create');
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, this can be simplified to a one-liner:

return methods.find(m => m.name === 'create');

@rashmihunt
Copy link
Contributor Author

@slnode test please

@rashmihunt
Copy link
Contributor Author

rashmihunt commented Aug 23, 2017 •
edited
Loading

loopback-connector-swagger failures are not related to this PR. 6 tests are failing on master branch for this connector for sometime. Opened an issue for this - strongloop/loopback-connector-swagger#45

@rashmihunt rashmihunt merged commit 3651c09 into master Aug 23, 2017
@rashmihunt rashmihunt deleted the updateonly_feature branch August 23, 2017 00:10
@rashmihunt rashmihunt removed the review label Aug 23, 2017
rashmihunt added a commit that referenced this pull request Aug 23, 2017
@rashmihunt
3.11.0
29e89f5 
 * Support createOnlyInstance in model (#3548) (Rashmi Hunt)
 * Add stalebot configuration (Kevin Delisle)
 * Catch errors on invalidate update (loay)
 * Update Issue and PR Templates (#3568) (Sakib Hasan)
@rashmihunt rashmihunt mentioned this pull request Aug 23, 2017
Closed
@kjdelisle kjdelisle mentioned this pull request Aug 23, 2017
Merged
@lehni lehni mentioned this pull request Oct 9, 2017
Merged
@bajtos bajtos mentioned this pull request Apr 26, 2018
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rashmihunt @bajtos