Add cmd get transactions by senderID and state filters - Closes #648 #651

ishantiw · 2018-11-19T14:58:43Z

Description

Add command to get transactions based on senderID, default, limit and offset values.

Also related to #600

Example,
lisk transaction:get --sender-id=14523483941411160698L --state=unsigned
lisk transaction:get 7397859286435869218 --state=unprocessed
lisk transaction:get --limit=10 --offset=15
lisk transaction:get

Review checklist

The PR resolves Add command to get all transactions by state with filters (senderId, limit or offset) #648
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

shuse2

Review for test not done yet

docs/transaction.md

src/commands/transaction/get.js

shuse2 · 2018-11-20T08:55:39Z

src/utils/query.js

@@ -26,7 +26,7 @@ export const handleResponse = (endpoint, res, placeholder) => {
 			}
 			throw new Error(`No ${endpoint} found using specified parameters.`);
 		}
-		return res.data[0];
+		return res.data;


wouldn't this affect other queries?

It was not returning a list of transactions from API so I had to change but I also tested all the commands which are using it and it works fine and also all the tests are passing.

I think test passes because we stub the API response, but it changes the format ='(

> ./bin/run account:get 123L,456L  [ { "0": { "address": "123L", "unconfirmedBalance": "6122216612", "balance": "6122216612", "publicKey": "", "secondPublicKey": "" } }, { "0": { "address": "456L", "unconfirmedBalance": "1000", "balance": "1000", "publicKey": "", "secondPublicKey": "" } } ]

src/commands/transaction/get.js

mitsuaki-u

tested ./bin/run transaction:get --senderId=13214966370334815499L --pretty --limit=3 and it seems to return transactions with senderId !== to provided id

mitsuaki-u · 2018-11-20T09:15:15Z

src/commands/transaction/get.js

+				return this.print(results);
+			}
+
+			if (senderAddress) {


should show some error if provided senderId does not exist or invalid. currently doesn't seem to throw.

That's a good catch, but actually right nowsenderId is made to work only with transaction state. So, if you don't use state field it will simply ignore senderId. But yes, we need to implement get transactions by senderId without state field too. I will update it.

ishantiw · 2018-11-20T16:37:41Z

Addressed above concerns.

Considering all the arguments and flags, below are some examples for testing

transaction:get 10041151099734832021
transaction:get 10041151099734832021,1260076503909567890
transaction:get 10041151099734832021,1260076503909567890 --state=unprocessed
transaction:get --state=unsigned --sender-id=1813095620424213569L
transaction:get 10041151099734832021 --state=unsigned --sender-id=1813095620424213569L
transaction:get --sender-id=1813095620424213569L
transaction:get --limit=10 --sort=amount:desc
transaction:get --limit=10 --offset=5

src/commands/transaction/get.js

test/commands/transaction/get.test.js

shuse2 · 2018-11-20T17:10:49Z

src/utils/query.js

@@ -26,7 +26,7 @@ export const handleResponse = (endpoint, res, placeholder) => {
 			}
 			throw new Error(`No ${endpoint} found using specified parameters.`);
 		}
-		return res.data[0];
+		return res.data;


I think test passes because we stub the API response, but it changes the format ='(

> ./bin/run account:get 123L,456L  [ { "0": { "address": "123L", "unconfirmedBalance": "6122216612", "balance": "6122216612", "publicKey": "", "secondPublicKey": "" } }, { "0": { "address": "456L", "unconfirmedBalance": "1000", "balance": "1000", "publicKey": "", "secondPublicKey": "" } } ]

mitsuaki-u

Looks good, mostly some changes to test suite format.

mitsuaki-u · 2018-11-21T09:01:55Z

test/commands/transaction/get.test.js

@@ -163,6 +158,109 @@ describe('transaction:get', () => {
 					return expect(printMethodStub).to.be.calledWithExactly(queryResult);
 				},
 			);
+		describe('transaction: get --sender-id', () => {


prefer empty line before describe blocks

mitsuaki-u · 2018-11-21T09:02:10Z

test/commands/transaction/get.test.js

+					return expect(printMethodStub).to.be.calledWithExactly(queryResult);
+				});
+		});
+		describe('transaction: get --limit --offset', () => {


same as above (empty line before)

mitsuaki-u · 2018-11-21T09:04:16Z

test/commands/transaction/get.test.js

+				.stub(api, 'default', sandbox.stub().returns(apiClientStub))
+				.stub(queryHandler, 'query', sandbox.stub().resolves(queryResult))
+				.command(['transaction:get', '--sender-id=12668885769632475474L'])
+				.it('should get all transactions info limited by limit value', () => {


update description to mention --sender-id flag

mitsuaki-u · 2018-11-21T09:05:50Z

test/commands/transaction/get.test.js

+				.stub(queryHandler, 'query', sandbox.stub().resolves(queryResult))
+				.command(['transaction:get'])
+				.it(
+					'should get all transactions based on default value of limit(20) and offset(0).',


description should be updated to limit(10)

mitsuaki-u · 2018-11-21T09:07:04Z

test/commands/transaction/get.test.js

+						return expect(printMethodStub).to.be.calledWithExactly(queryResult);
+					},
+				);
+			setupTest()


same as above (empty line before)

mitsuaki-u · 2018-11-21T09:14:22Z

test/commands/transaction/get.test.js

-					]);
-				});
+				.it(
+					'should get a transaction’s info and display as an array for a state.',


should get an unprocessed transaction’s info and display as an array.

mitsuaki-u · 2018-11-21T09:14:45Z

test/commands/transaction/get.test.js

 				])
 				.it(
-					'should get transactions info only using non-empty args and display as an array',
+					'should get transaction’s info and display as an array for a state.',


should get an unsigned transaction’s info and display as an array.

mitsuaki-u · 2018-11-21T09:16:19Z

test/commands/transaction/get.test.js

 				])
 				.it(
-					'should get transactions info only using non-empty args and display as an array',
+					'should get a transaction’s info for a given txn Id, senders address and state and display as an array.',


sender's address

mitsuaki-u · 2018-11-21T09:22:40Z

test/commands/transaction/get.test.js

+				.stub(api, 'default', sandbox.stub().returns(apiClientStubNode))
+				.command(['transaction:get', '--state=unprocessed', '--limit=10'])
+				.it(
+					'should get transactions for a given state taking limit or offset or default.',


should get transactions for a given state without specified txn id.

mitsuaki-u · 2018-11-21T09:25:47Z

test/commands/transaction/get.test.js

@@ -194,60 +292,94 @@ describe('transaction:get', () => {
 			},
 		};

-		describe('transaction: get transactions --state-unprocessed', () => {
+		setupTest()


Do we need to add queryHandler expects for all tests in this test suite below(transaction:get transactions --state)

For all queries related to getting transactions by a state is not using the general query handler but its own handler so it's not used for these test suits.

Ok I think we should also test for custom queryNode handler in the same way

@mitsujutsu discussed with @shuse2, I will add it in testing

ishantiw · 2018-11-21T10:59:58Z

@shuse2 fixed query handler, updated it to handle arrays more than 1 records. This time I tested it with the right build 😅 but please test it again on your end also.

@mitsujutsu addressed 👍

mitsuaki-u · 2018-11-22T10:10:56Z

test/commands/transaction/get.test.js

 			},
 		];

 		setupTest()
 			.stub(api, 'default', sandbox.stub().returns(apiClientStub))
 			.stub(queryHandler, 'query', sandbox.stub().resolves(queryResult))
 			.command(['transaction:get', transactionIds.join(',')])
-			.it('should get two transactions’ info and display as an array', () => {
+			.it('should get two transactions’ info and display as an array.', () => {


transactions’ --> transaction's

mitsuaki-u

one missing test

mitsuaki-u · 2018-11-22T10:23:32Z

test/commands/transaction/get.test.js

+			.command(['transaction:get', transactionId, '--state=unprocessed'])
+			.it(
+				'should get an unprocessed transaction’s info and display as an array.',
+				() => {


should this test also have queryNodeTransaction expect?

ishantiw added 4 commits November 19, 2018 15:36

🌱 Add get trx cmd by senderid and filter

fd5e5da

✅ New tests for get trx cmd

1225b78

♻️ Update query handler and related test

46f446c

📚 Update docs for get trx flags

ee7415e

ishantiw added the 🏗️ in progress label Nov 19, 2018

ishantiw added this to the Version 2.1.0 milestone Nov 19, 2018

ishantiw self-assigned this Nov 19, 2018

ishantiw requested review from shuse2 and mitsuaki-u November 19, 2018 14:58

ishantiw changed the title ~~648 add cmd get transactions by state filters~~ Add cmd get transactions by senderID and state filters. Closes #648 Nov 19, 2018

ishantiw changed the title ~~Add cmd get transactions by senderID and state filters. Closes #648~~ Add cmd get transactions by senderID and state filters - Closes #648 Nov 19, 2018

ishantiw added 👁️ pending review and removed 🏗️ in progress labels Nov 19, 2018

shuse2 suggested changes Nov 20, 2018

View reviewed changes

mitsuaki-u suggested changes Nov 20, 2018

View reviewed changes

ishantiw added 2 commits November 20, 2018 17:25

🌱 Add sort flag and update cases

d3a5922

📚 Update docs for get txns

61c74e7

shuse2 suggested changes Nov 20, 2018

View reviewed changes

mitsuaki-u suggested changes Nov 21, 2018

View reviewed changes

ishantiw added 3 commits November 21, 2018 10:59

🐛 Fix query handler for return type

ffbb75a

💅 Fix tests description and spacing

6c3a45a

✅ Add test for updated query handler

4bb3b4f

♻️ Move query handler to global

3128398

shuse2 previously approved these changes Nov 22, 2018

View reviewed changes

mitsuaki-u reviewed Nov 22, 2018

View reviewed changes

mitsuaki-u suggested changes Nov 22, 2018

View reviewed changes

✅ Add and update tests for node query handler

adfb8bf

ishantiw dismissed shuse2’s stale review via adfb8bf November 22, 2018 13:30

mitsuaki-u approved these changes Nov 22, 2018

View reviewed changes

shuse2 approved these changes Nov 22, 2018

View reviewed changes

shuse2 added ✅ ready and removed 👁️ pending review labels Nov 22, 2018

shuse2 merged commit 838ee16 into development Nov 22, 2018

shuse2 deleted the 648-add-cmd-get-transactions-by-state-filters branch November 22, 2018 14:15

shuse2 removed this from the Version 2.1.0 milestone Nov 28, 2018

ishantiw mentioned this pull request Dec 7, 2018

Add flag for unprocessed/unsigned tx - Closes #110, #139 LiskArchive/lisk-legacy-docs#140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cmd get transactions by senderID and state filters - Closes #648 #651

Add cmd get transactions by senderID and state filters - Closes #648 #651

ishantiw commented Nov 19, 2018 •

edited by shuse2

Loading

shuse2 left a comment

shuse2 Nov 20, 2018

ishantiw Nov 20, 2018

shuse2 Nov 20, 2018

mitsuaki-u left a comment

mitsuaki-u Nov 20, 2018

ishantiw Nov 20, 2018

ishantiw commented Nov 20, 2018

shuse2 Nov 20, 2018

mitsuaki-u left a comment

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

mitsuaki-u Nov 21, 2018

ishantiw Nov 21, 2018

mitsuaki-u Nov 21, 2018

ishantiw Nov 21, 2018

ishantiw commented Nov 21, 2018

mitsuaki-u Nov 22, 2018

mitsuaki-u left a comment

mitsuaki-u Nov 22, 2018

Add cmd get transactions by senderID and state filters - Closes #648 #651

Add cmd get transactions by senderID and state filters - Closes #648 #651

Conversation

ishantiw commented Nov 19, 2018 • edited by shuse2 Loading

Description

Review checklist

shuse2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitsuaki-u left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw commented Nov 20, 2018

Choose a reason for hiding this comment

mitsuaki-u left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw commented Nov 21, 2018

Choose a reason for hiding this comment

mitsuaki-u left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw commented Nov 19, 2018 •

edited by shuse2

Loading