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.

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

Account.get() does not work when multiple filters are provided with same key - Closes #2822 #2836

Merged
merged 5 commits into from Feb 13, 2019

Conversation

Projects
5 participants
@SargeKhan
Copy link
Member

commented Feb 6, 2019

What was the problem?

The parseFilter function defined in BaseEntity class were not able to handle filters as input when it's an array with the same property name.

How did I fix it?

Fixed the bug by refactoring the code.

How to test it?

Run unit tests.

Review checklist

  • The PR resolves #2822
  • All new code is covered with unit tests
  • All new code was formatted with Prettier
  • Linting passes
  • Tests pass
  • Commit messages follow the commit guidelines
  • Documentation has been added/updated

SargeKhan added some commits Feb 6, 2019

@SargeKhan SargeKhan self-assigned this Feb 7, 2019

@MaciejBaj MaciejBaj requested a review from pablitovicente Feb 7, 2019

@MaciejBaj MaciejBaj added this to In progress in Version 1.6.0 via automation Feb 7, 2019

Version 1.6.0 automation moved this from In progress to Pending Review Feb 7, 2019

@SargeKhan SargeKhan dismissed stale reviews from lsilvs and pablitovicente via 0683b95 Feb 8, 2019

@nazarhussain
Copy link
Contributor

left a comment

Logically that's not an issue actually because the use case can implemented with _in query.

Account.get({address_in: ['a', 'b']})

or

Account.get([{address: 'a'}, {address: 'b'}])

Both have the same effect. I suggest to use to first approach and implement your logic on your end.

But if there is a real necessary and first approach is not possible at all, then you can use the second.

@SargeKhan

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@nazarhussain, in transactions element, we don't want to use SQL related filters like in or like. Therefore, we have gone with the second approach.

@MaciejBaj MaciejBaj merged commit 4cdeb52 into development Feb 13, 2019

4 checks passed

coverage/coveralls Coverage decreased (-11.9%) to 78.604%
Details
jenkins-ci/lisk-core This commit looks good
Details
jenkins-ci/lisk-core-network This commit looks good
Details
security/snyk - package.json (LiskHQ) No new issues
Details

Version 1.6.0 automation moved this from Pending Review to Closed PRs Feb 13, 2019

@MaciejBaj MaciejBaj deleted the 2822-parse_filters_bug branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.