Skip to content

[WIP]Expose replace methods for related models - #1616

Closed
ivanschwarz wants to merge 3 commits into
loopbackio:masterfrom
ivanschwarz:feat/expose-replace-methods-for-related-models
Closed

[WIP]Expose replace methods for related models#1616
ivanschwarz wants to merge 3 commits into
loopbackio:masterfrom
ivanschwarz:feat/expose-replace-methods-for-related-models

Conversation

@ivanschwarz

Copy link
Copy Markdown

Description

This PR is a prerequisite for strongloop/loopback#3959. You can find details there.

@bajtos : Can you review?

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@slnode

slnode commented Aug 3, 2018

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos bajtos left a comment

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.

LGTM.

@strongloop/sq-lb-apex can I get another pair of eyes to review these changes please and merge the pull request if all looks good?

@bajtos

bajtos commented Aug 13, 2018

Copy link
Copy Markdown
Member

@slnode ok to test

@ivanschwarz

Copy link
Copy Markdown
Author

@bajtos : Thank you for your review. I think we should wait with merging this PR. Like I noted in strongloop/loopback#3959, I've implemented changes just for hasMany relation. I think I should extend this functionality to all other relevant relations. Essentially every PUT endpoint should eventually have its matching PATCH endpoint. I just wanted to have a positive feedback on my approach first to avoid unnecessary work.

@jannyHou jannyHou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ivanschwarz Thank you for working on exposing the PUT method for relations!
Your code change looks reasonable to me 👍
Just some linting errors, you can also check them by running npm run lint on local:

09:22:47 > eslint .
09:22:47 
09:23:13 
09:23:13 /home/jenkins/workspace/nb/loopback-datasource-juggler~master/7a48ad0e/test/model-inheritance.test.js
09:23:13   76:1  error  Expected indentation of 6 spaces but found 8  indent
09:23:13 
09:23:13 ✖ 1 problem (1 error, 0 warnings)
09:23:13   1 error and 0 warnings potentially fixable with the `--fix` option.
09:23:13 

Comment thread test/relations.test.js

it('should replace scoped record with promises', function(done) {
var id;
Book.create()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick(Not a big deal feel free to ignore): To simplify the test code I think you can omit done here.

it('should replace...', function() {
  return Book.create()
    .then(function(book) {
      // create chapters
    })
    .then(function(ch) {
       return verify(book);
     });

    function verify(book) {
      return book.chapters.findById(id)
        .then(function(ch) {
          // verify chapter
         });
    }
})

@raymondfeng

Copy link
Copy Markdown
Contributor

@jannyHou PTAL

@bajtos bajtos added the community-contribution Patches contributed by community label Mar 19, 2019
@stale

stale Bot commented Jul 11, 2019

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale label Jul 11, 2019
@stale

stale Bot commented Jul 25, 2019

Copy link
Copy Markdown

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale Bot closed this Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Patches contributed by community stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants