Add a member to an org by MeBrei · Pull Request #450 · radicle-dev/radicle-registry

MeBrei · 2020-05-13T11:17:18Z

Add the functionality to add a member to an existing org.

Todos:

add more runtime tests
- bad author scenario, where the said author would pay the fee
- no org scenario, where the said author would pay the fee
- no user associated scenario, where the said author would pay the fee
add end-to-end tests
add cli functionality
error out if an existing member is added again

In a follow up, the functionality to remove a member should be added

NunoAlexandre

My preemptive look 👍

Going great so far!

One remark: The new message is called RegisterMember, which I find confusing. It's not really registering a user, it's adding an existing user to the members of an org. I would, therefore, prefer to call it "AddOrgMember", which will also be less likely to collide with future messages. Note how the fn you added to Org is named add_member already.

Edit: @cloudhead I see that the spec calls it comes from the spec, 3.3 register-member.

client/src/message.rs

NunoAlexandre · 2020-05-13T11:29:57Z

core/src/message.rs

    pub user_id: Id,
 }

+/// Register a member of an org on the Radicle Registry with the given ID.


Suggested change

/// Register a member of an org on the Radicle Registry with the given ID.

/// Register a member of an org with the given ID.

It is unclear which entity the ID is referring to though, in both versions.

This is why I advised to name it handle to make it clear that it is a user.

core/src/message.rs

NunoAlexandre · 2020-05-13T11:32:52Z

core/src/message.rs

+///
+#[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)]
+pub struct RegisterMember {
+    // The member to register, unique in the org.


Is the "unique in the org" necessary? For a moment I wondered if that was actually a state-dependent validation. Is it? If it isn't, then I would drop this bit here since Ids are already unique in nature. If it is, then we should have that under the state-dependent validations above.

Isn't this to express that the member can only be registered once per org? Making the tuple (org_id, user_handle) unique?

it's referring to the member being unique within the org, ie a member can't be added twice

runtime-tests/tests/member_registration.rs

runtime/src/registry.rs

test-utils/src/lib.rs

xla

Great stuffz! Would distinct handle from other ids and more inline.

core/src/message.rs

core/src/state.rs

runtime/src/registry.rs

test-utils/src/lib.rs

xla · 2020-05-13T12:46:18Z

One remark: The new message is called RegisterMember, which I find confusing. It's not really registering a user, it's adding an existing user to the members of an org. I would, therefore, prefer to call it "AddOrgMember", which will also be less likely to collide with future messages. Note how the fn you added to Org is named add_member already.

Edit: @cloudhead I see that the spec calls it comes from the spec, 3.3 register-member.

Semantically registering a member is correct, as it is with org, user and (org/user, projectname) a unique entry in the registry. We don't need to qualify with Org here as there is no other concept of member and the add_member fn is for adding to the actual list, not about the external API.

NunoAlexandre · 2020-05-13T14:00:11Z

One remark: The new message is called RegisterMember, which I find confusing. It's not really registering a user, it's adding an existing user to the members of an org. I would, therefore, prefer to call it "AddOrgMember", which will also be less likely to collide with future messages. Note how the fn you added to Org is named add_member already.
Edit: @cloudhead I see that the spec calls it comes from the spec, 3.3 register-member.

Semantically registering a member is correct, as it is with org, user and (org/user, projectname) a unique entry in the registry. We don't need to qualify with Org here as there is no other concept of member and the add_member fn is for adding to the actual list, not about the external API.

I don't disagree with you, but present "register member" to an org vs "add member" to an org to any number of users and I am pretty sure which one will be simpler to understand.

xla · 2020-05-13T14:07:29Z

I don't disagree with you, but present "register member" to an org vs "add member" to an org to any number of users and I am pretty sure which one will be simpler to understand.

In this context we are talking about the semantics of the chain and not what is presented to the user in the UI.

NunoAlexandre · 2020-05-13T14:56:28Z

I don't disagree with you, but present "register member" to an org vs "add member" to an org to any number of users and I am pretty sure which one will be simpler to understand.

In this context we are talking about the semantics of the chain and not what is presented to the user in the UI.

I know :) But why have two terminologies when "add member" works for both audiences? While registering an org / project / user actually registers new entities into the chain, "register a member" simply links two entities that are already registered.

xla · 2020-05-19T08:13:18Z

Part of radicle-dev/radicle-upstream#349

MeBrei · 2020-05-19T14:37:58Z

I don't disagree with you, but present "register member" to an org vs "add member" to an org to any number of users and I am pretty sure which one will be simpler to understand.

In this context we are talking about the semantics of the chain and not what is presented to the user in the UI.

I know :) But why have two terminologies when "add member" works for both audiences? While registering an org / project / user actually registers new entities into the chain, "register a member" simply links two entities that are already registered.

How can we come to a decision on this discussion? @NunoAlexandre @xla @cloudhead

NunoAlexandre

Looking great, @MeBrei! 🚀

I left some comments but mostly small stuff.

NunoAlexandre · 2020-05-20T08:47:54Z

cli/src/command/org.rs

    Register(Register),
    /// Unregister an org.
    Unregister(Unregister),
+    /// Register a new member under the org.


Suggested change

/// Register a new member under the org.

/// Register a new member under an org.

NunoAlexandre · 2020-05-20T08:49:43Z

cli/src/command/org.rs

+
+        register_member_fut.await?.result?;
+        println!(
+            "✓ User {} is now registered under Org {}.",


Suggested change

"✓ User {} is now registered under Org {}.",

"✓ User {} is now a member of the Org {}.",

NunoAlexandre · 2020-05-20T08:52:36Z

client/tests/end_to_end.rs

+    assert_eq!(org_registered_tx.result, Ok(()));
+
+    // The org needs funds to submit transactions.
+    let org = client.get_org(org_id.clone()).await.unwrap().unwrap();


since we already get the org here, it wouldn't hurt to assert that it does not contain user_id in its list of members at this point.

NunoAlexandre · 2020-05-20T08:54:37Z

client/tests/end_to_end.rs

+    );
+
+    let org: Org = client.get_org(org_id.clone()).await.unwrap().unwrap();
+    assert!(


I would still prefer the check here to be more specific and check that the org now has two members, the author user which registered it and this new user_id.

NunoAlexandre · 2020-05-20T08:55:45Z

core/src/error.rs


+    #[cfg_attr(
+        feature = "std",
+        error("a member with the same ID already exists within the org.")


suggestion:

Suggested change

error("a member with the same ID already exists within the org.")

error("already a member of the org")

to be clear, this was just a suggestion, since DuplicateMember could reflect that the vec Org::members has duplicates, when the error is to say "you are trying to add a user to the members of an org but they are already a member".

That said, if we go ahead with this change, I'd suggest extending the error message to this, (sorry for the double comment):

error("the user is already a member of the org")

NunoAlexandre · 2020-05-20T09:00:28Z

runtime-tests/tests/member_registration.rs

+    };
+    let tx_applied = submit_ok(&client, &author, message.clone()).await;
+
+    assert_eq!(tx_applied.result, Err(RegistryError::InexistentOrg.into()));


We need to check that author has paid the tx fee

runtime-tests/tests/member_registration.rs

NunoAlexandre · 2020-05-20T09:02:40Z

runtime-tests/tests/member_registration.rs

+}
+
+#[async_std::test]
+async fn register_nonexistent_member() {


I got confused with the name for a sec, maybe:

Suggested change

async fn register_nonexistent_member() {

async fn register_nonexistent_user() {

NunoAlexandre · 2020-05-20T09:04:58Z

runtime/src/registry.rs

+                return Err(RegistryError::InsufficientSenderPermissions.into());
+            }
+
+            if None == store::Users::get(message.user_id.clone()) {


Suggested change

if None == store::Users::get(message.user_id.clone()) {

if store::Users::get(message.user_id.clone()).is_none() {

NunoAlexandre · 2020-05-20T09:06:11Z

runtime/src/registry.rs

+                return Err(RegistryError::DuplicateMember.into());
+            }
+
+            store::Orgs::insert(message.org_id.clone(), org.add_member(message.user_id.clone()));


We mostly try to separate such lines, like so:

Suggested change

store::Orgs::insert(message.org_id.clone(), org.add_member(message.user_id.clone()));

let org_with_member = org.add_member(message.user_id.clone());

store::Orgs::insert(message.org_id.clone(), org_with_member);

NunoAlexandre · 2020-05-20T09:08:42Z

runtime/src/registry.rs

    pub enum Event {
        CheckpointCreated(CheckpointId),
        CheckpointSet(ProjectName, Id, CheckpointId),
+        MemberRegistered(Id, Id),


Forgot this one! Since the types are ambiguous, I'd rather change this to:

Suggested change

MemberRegistered(Id, Id),

MemberRegistered { user_id: Id, org_id: Id },

seems like the macro does not support variants with named fields :/

geigerzaehler · 2020-05-20T10:02:49Z

@MeBrei: Could you ping me once this is ready? I’d also like to have a brief look at this

NunoAlexandre

Awesome 🚀

just a comment left from me: #450 (comment)

geigerzaehler

Great work 🎆 Just two requests to make the documentation comprehensive.

geigerzaehler · 2020-05-20T10:12:06Z

core/src/message.rs

+///
+/// The identified org must exit.
+///
+/// A user associated with the author must exist.


Two validations are missing

The user user_id must exist

user_id must not already be a member of the org.

Two validations are missing

The user user_id must exist

It says already:

A user associated with the author must exist.

we could change that to:

A user associated with the author must exist and be identified by user_id

Isn’t the user associated with the author different from user_id?

yes, will change it.

changed in 8322c12

geigerzaehler · 2020-05-20T10:15:42Z

core/src/message.rs


+/// Register a new member for an org on the Registry with the given user ID.
+///
+/// # State-dependent validations


Could you also list the state changes?

/// # State changes /// If successful, `user_id` is added to [Org::members] of `org_id`.

added in 8322c12

xla

🔹 💜 ⤴️ 💈

geigerzaehler

👏

Co-authored-by: Nuno Alexandre <hi@nunoalexandre.com> Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

MeBrei requested a review from xla May 13, 2020 11:17

xla changed the base branch from master to upstream May 13, 2020 11:19

MeBrei self-assigned this May 13, 2020

NunoAlexandre reviewed May 13, 2020

View reviewed changes

xla suggested changes May 13, 2020

View reviewed changes

MeBrei marked this pull request as ready for review May 20, 2020 07:45

NunoAlexandre self-requested a review May 20, 2020 08:47

NunoAlexandre reviewed May 20, 2020

View reviewed changes

geigerzaehler self-requested a review May 20, 2020 09:59

NunoAlexandre approved these changes May 20, 2020

View reviewed changes

geigerzaehler reviewed May 20, 2020

View reviewed changes

xla approved these changes May 20, 2020

View reviewed changes

geigerzaehler approved these changes May 20, 2020

View reviewed changes

Add register member under an org functionality

59b7fa0

Co-authored-by: Nuno Alexandre <hi@nunoalexandre.com> Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

MeBrei force-pushed the merle/add-member branch from 61a33a8 to 59b7fa0 Compare May 20, 2020 16:02

MeBrei merged commit 2bf1b82 into upstream May 20, 2020

MeBrei deleted the merle/add-member branch May 20, 2020 16:15

This was referenced May 22, 2020

Fix failing tests on upstream branch #467

Closed

Increment spec version and recompile wasm #468

Merged

	/// Register a member of an org on the Radicle Registry with the given ID.
	/// Register a member of an org with the given ID.

	/// Register a new member under the org.
	/// Register a new member under an org.

	"✓ User {} is now registered under Org {}.",
	"✓ User {} is now a member of the Org {}.",

	error("a member with the same ID already exists within the org.")
	error("already a member of the org")

	async fn register_nonexistent_member() {
	async fn register_nonexistent_user() {

	if None == store::Users::get(message.user_id.clone()) {
	if store::Users::get(message.user_id.clone()).is_none() {

	store::Orgs::insert(message.org_id.clone(), org.add_member(message.user_id.clone()));
	let org_with_member = org.add_member(message.user_id.clone());
	store::Orgs::insert(message.org_id.clone(), org_with_member);

	MemberRegistered(Id, Id),
	MemberRegistered { user_id: Id, org_id: Id },

Conversation

MeBrei commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NunoAlexandre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xla commented May 13, 2020

Uh oh!

NunoAlexandre commented May 13, 2020

Uh oh!

xla commented May 13, 2020

Uh oh!

NunoAlexandre commented May 13, 2020

Uh oh!

xla commented May 19, 2020

Uh oh!

MeBrei commented May 19, 2020

Uh oh!

NunoAlexandre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NunoAlexandre May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geigerzaehler commented May 20, 2020

MeBrei commented May 13, 2020 •

edited

Loading

NunoAlexandre left a comment •

edited

Loading

NunoAlexandre May 20, 2020 •

edited

Loading