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

Showing MSP admins present in wallet while updating MSP definition #525

Merged
merged 7 commits into from
Sep 19, 2023

Conversation

selvaprakash92
Copy link
Contributor

@selvaprakash92 selvaprakash92 commented Sep 8, 2023

Type of change

  • New feature

Description

This PR shows/lists the identities from the local wallet that are also admins on the channel, so that the user can make a better informed choice when selecting the identity to use when changing the MSP definition.

Prakash P S added 2 commits September 7, 2023 18:26
Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
@selvaprakash92 selvaprakash92 requested a review from a team as a code owner September 8, 2023 01:21
Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

couple things

  • using a type: 'text' field for this doesn't feel right, use a type: 'component', see screen shot
  • add a tooltip, see screen shot
  • change label to Known channel admins
  • remove the index < 1 if statement, always show the label
  • the big change, is you need to actually filter the wallet identities, right now the fields you added are identical to the Identity dropdown, which isn't helpful, it should be built off the channel's config block admin section and then compared to the identities in the dropdown. the list of identities should be the admin identities for this MSP on this channel.
  • change the local onClose function to some other name, it's someimtes confusing to have the same function name in two different scopes (that are right next to eachother)
  • lastly fix any linting errors, so far you have a missing semicolon
  • oh also, edit the description of this PR, needs a better description, too vague

shoot to make this: (note I named my identities in this example admin and tls-admin)
image

Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
@selvaprakash92 selvaprakash92 changed the title Showing admins Showing MSP admins present in wallet while updating MSP definition Sep 12, 2023
Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

better! it's getting there, see comments

@@ -343,6 +371,20 @@ class UpdateChannelMspModal extends React.Component {
return fields;
}

populateMSPAdmins()
Copy link
Contributor

@dshuffma-ibm dshuffma-ibm Sep 12, 2023

Choose a reason for hiding this comment

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

this is close, but not quite what i had in mind. instead of looking at the node ou in the certs in the wallet, look in the config block for this channel, and find the MSP section and its root certs, then compare these certs to the ones in the wallet, if they match, add the wallet identity's name to the list

you will need to reach back a bit because UpdateChannelMspModal doesn't have this data (the config block), and it's parent ChannelMembers doesn't have it either, but that guy's parent ChannelDetails does. so just feed it down as arguments to each component until UpdateChannelMspModal can see the data you need

Copy link
Contributor

Choose a reason for hiding this comment

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

@selvaprakash92 this part has not been done

Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
@selvaprakash92
Copy link
Contributor Author

selvaprakash92 commented Sep 13, 2023

@dshuffma-ibm , updated the style and label suggestions,

regarding logic:
on the populateMSPAdmins function, I am using the object submit_identity_options which is getting generated by matching the MSP root cert with wallet identities.

submit_identity_options = await IdentityApi.getIdentitiesForMsp(new_msp_definition);
.

on backtracking this, it is getting the results via

mspIdentities = await IdentityApi.getIdentitiesForCerts(certs);

from:

let isIdentityFromCert = await StitchApi.isIdentityFromRootCert({

Since it is returning all type of identities with that root cert, I am filtering it with OU: admin to show only admin identities

Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

still needs logic changes to the match identities to the * channel's * config block msp section

the data you are currently using is just referencing local msp data with local wallet data, it is not referencing the channel's config block, which is what is needed

@@ -343,6 +371,20 @@ class UpdateChannelMspModal extends React.Component {
return fields;
}

populateMSPAdmins()
Copy link
Contributor

Choose a reason for hiding this comment

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

@selvaprakash92 this part has not been done

Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
@selvaprakash92
Copy link
Contributor Author

@dshuffma-ibm , Updated the logic to fetch the msp from channel config. And match the root and intermediate certs of that to the wallet to show the identities

Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

almost, the cert matching logic looks good, its comparing the orgs for the msp section of the config block with the local identities, however I don't like fetching the config block when we already have the block. see my earlier comment:

you will need to reach back a bit because UpdateChannelMspModal doesn't have this data (the config block), and it's parent ChannelMembers doesn't have it either, but that guy's parent ChannelDetails does. so just feed it down as arguments to each component until UpdateChannelMspModal can see the data you need

if you already attempted this ^^ but didn't get it to work, i'd like to know what happened

Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
@selvaprakash92
Copy link
Contributor Author

@dshuffma-ibm , I have implemented the changes as per review comments, now I am getting the data from ChannelDetails via ChannelMembers to UpdateChannelMspModel

Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

looks good, thanks for all the changes

@dshuffma-ibm dshuffma-ibm merged commit 56f9a2f into hyperledger-labs:main Sep 19, 2023
17 checks passed
@selvaprakash92 selvaprakash92 deleted the showing-admins branch October 12, 2023 05:48
dshuffma-ibm pushed a commit to dshuffma-ibm/fabric-operations-console that referenced this pull request Oct 17, 2023
…yperledger-labs#525)

* initial draft on showing admins
* updated the logic to get msp from channel config
---------
Signed-off-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
Co-authored-by: Prakash P S <prakashps@Prakashs-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants