Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Get All/Resolve All Feature for Retrieve Action, issue #76 #78

Merged
merged 6 commits into from
Apr 24, 2018
Merged

Get All/Resolve All Feature for Retrieve Action, issue #76 #78

merged 6 commits into from
Apr 24, 2018

Conversation

mwittig
Copy link
Contributor

@mwittig mwittig commented Nov 28, 2017

See issue #76

@mwittig
Copy link
Contributor Author

mwittig commented Nov 28, 2017

Sorry, I wasn't aware of the required DCO claim. I have updated the commit message accordingly.

@cazfletch
Copy link
Contributor

@mwittig I think you might still have DCO sign off problems

@mwittig mwittig force-pushed the master branch 2 times, most recently from fb6d855 to e988467 Compare November 28, 2017 20:51
@mwittig
Copy link
Contributor Author

mwittig commented Nov 28, 2017 •
edited

I have changed the sign-off messages to make DCO robot happy. In my opinion the robot check is too rigid as it does a case sensitive match for the e-mail address. Moreover it should not check the user name as it maps to my local git config user.name. I can change it at any point in time as it is not part of the Github user profile. I have just digged a little deeper. Now, I understand why DCO robot works as is. Actually, the local part of the e-mail address is case sensitive even though this is discouraged according to section 2.4 of RFC 5321. Regarding the user name part the matter has been recently discussed as part of DCO robot project.

@cazfletch cazfletch self-requested a review November 29, 2017 09:48
cazfletch
cazfletch suggested changes Nov 29, 2017
View reviewed changes
Copy link
Contributor

@cazfletch cazfletch left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I've left a couple of comments. Could you also add some info to the README.md to show how to use this feature please.

packages/node-red-contrib-composer/nodes/hyperledger-composer.js Outdated
@@ -213,7 +223,12 @@ module.exports = function (RED) {
return businessNetworkConnection.getParticipantRegistry(modelName)
.then((participantRegistry) => {
node.log('got participant registry');
return participantRegistry.get(id);
if (id == null) {
return assetRegistry.getAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be partcipantRegistry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. It's a stupid copy-paste error. I'll fix that as part of the next commit

packages/node-red-contrib-composer/nodes/hyperledger-composer.js Outdated
@@ -213,7 +223,12 @@ module.exports = function (RED) {
return businessNetworkConnection.getParticipantRegistry(modelName)
.then((participantRegistry) => {
node.log('got participant registry');
return participantRegistry.get(id);
if (id == null) {
return assetRegistry.getAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also do a resolveAll() on a participant registry too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. The current release does not handle resolve case for participant retrieval and I thought in the first place it is perhaps not required for participant objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's probably a bug so if you could add it as part of your implementation that would be really good

@mwittig mwittig force-pushed the master branch 2 times, most recently from 34af6e1 to 58dec66 Compare December 4, 2017 10:51
@mwittig
Copy link
Contributor Author

mwittig commented Dec 4, 2017

Please give some more time to revise the README and the help node texts.

@cazfletch
Copy link
Contributor

@mwittig any update on these changes?

@mwittig
Copy link
Contributor Author

mwittig commented Mar 6, 2018

@caroline-church Sorry for the delay at my end. I promise to finish up asap. As I am travelling this week I am shooting for early next week.

@cazfletch
Copy link
Contributor

@mwittig It has been month since your last update, are you intending to update your pull request?

@mwittig
added "get all" feature to retrieve action if payload has no id property
28833bc 
Signed-off-by: marcus <wittigmarcus@gmail.com>
@mwittig
removed id check for retrieve action
cf10413 
Signed-off-by: marcus <wittigmarcus@gmail.com>
@mwittig
use getAll instead of resolveAll for retrieval of participants
ea03f8b 
Signed-off-by: marcus <wittigmarcus@gmail.com>
@mwittig
implemented resolve feature for participant objects as part of retrie…
3610f7c 
…ve action

Signed-off-by: marcus <wittigmarcus@gmail.com>
@mwittig
formatting changes to have headings rendered properly on github
543cf0e 
Signed-off-by: marcus <wittigmarcus@gmail.com>
@mwittig
added node info text for getAll/resolveAll feature
18d28ac 
Signed-off-by: marcus <wittigmarcus@gmail.com>
@mwittig
Copy link
Contributor Author

mwittig commented Apr 24, 2018

Sorry for the delay. I think I have covered all your review comments. Regarding your request to "add some info to the README.md to show how to use this feature" I have decided to add some text to the info for the Hyperledger-Composer-Mid node instead. I think the README pretty generic and is better to have the specific information available as part of the editor info view. Hope, this ok.

@cazfletch cazfletch merged commit 50c6643 into hyperledger-archives:master Apr 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cazfletch cazfletch cazfletch approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@mwittig @cazfletch