Fix/4823 validator profile #4849

chrlschwb · 2023-08-29T13:15:06Z

thesan

The mapping looks good. Please reset the changes to the .yarnrc.yml and query-node/schemas/content.graphql

Also please test the mapping in tests/network-tests/src/flows/membership/updatingProfile.ts

thesan · 2023-08-29T16:18:32Z

query-node/schemas/membership.graphql

@@ -47,6 +47,9 @@ type MemberMetadata @entity {

  "Social media handles, email address..."
  externalResources: [MembershipExternalResource] @derivedFrom(field: "memberMetadata")
+
+  isVerified: Boolean!


Since this is not in a separate entity anymore. Please rename it to something more explicit like isVerifiedValidator

thesan · 2023-08-29T16:27:04Z

query-node/mappings/src/membership.ts

+  if (typeof metadata?.validatorAccount === 'string') {
+    member.metadata.validatorAccount = (metadata.validatorAccount || null) as string


This could be:

Suggested change

if (typeof metadata?.validatorAccount === 'string') {

member.metadata.validatorAccount = (metadata.validatorAccount || null) as string

if (typeof metadata?.validatorAccount === 'string' && metadata.validatorAccount !== member.metadata.validatorAccount) {

member.metadata.validatorAccount = metadata.validatorAccount || undefined

To prevent resetting the verification by mistake when the same account is passed and to allow removing the validator account by passing an empty string.

removing null creates error during membership update test where validatorAccount should be set to null

Nice catch ! I'm not sure of this but maybe null actually unset the value on hydra while undefined is ignored (I think I read an issue about something like that a while ago).

|

thesan

The mappings and schema are great now but as I mentioned in the last review please add a test to the membership updating profile flow.

To run the tests locally you can follow the query node README:

In .env uncomment JOYSTREAM_NODE_TAG and set it to the latest testing image, currently it's: 44832c0fe83c518779d52b5b3a2dcce94ae2d805 (do not commit this change.
Run the ./query-node/build.sh script to build the schema and mappings locally.
Try running DEBUG=true ./query-node/run-tests.sh memberships. Hopefully it works otherwise you will have to run the full scenario all the time with DEBUG=true ./query-node/run-tests.sh (which is more time consuming).
After running the tests once in (1) you might to modify them and re-run the a lot faster with REUSE_KEYS=true yarn workspace network-tests run-test-scenario memberships

If you change the mappings or the schema, you will have to redo (2) and (3).

You can check this commit for more details and ask me if you get stuck.

thesan

You can reset the yarn.lock by running git checkout master -- yarn.lock and committing the changes.

thesan · 2023-09-04T15:03:09Z

metadata-protobuf/proto/Membership.proto

  repeated ExternalResource externalResources = 5;
+
+  optional bool isVerifiedValidator = 6;


I just realized there shouldn't be any isVerifiedValidator here. The MembershipMetadata can only define a validatorAccount but not whether or not it's verified.

thesan · 2023-09-04T15:04:04Z

tests/network-tests/src/fixtures/membership/UpdateProfileHappyCaseFixture.ts

@@ -16,6 +16,8 @@ export type MemberProfileData = {
  about?: string | null
  avatarUri?: string | null
  externalResources?: MembershipMetadata.IExternalResource[] | null
+  isVerifiedValidator?: boolean


Same here

Suggested change

isVerifiedValidator?: boolean

tests/network-tests/src/fixtures/membership/UpdateProfileHappyCaseFixture.ts

thesan · 2023-09-04T15:18:48Z

tests/network-tests/src/fixtures/membership/UpdateProfileHappyCaseFixture.ts

+      isVerifiedValidator: isSet(this.newValues.isVerifiedValidator)
+        ? this.newValues.isVerifiedValidator
+        : this.oldValues.isVerifiedValidator,


So I don't think this is needed

Suggested change

isVerifiedValidator: isSet(this.newValues.isVerifiedValidator)

? this.newValues.isVerifiedValidator

: this.oldValues.isVerifiedValidator,

thesan · 2023-09-04T15:20:35Z

tests/network-tests/src/flows/membership/updatingProfile.ts

+      isVerifiedValidator: false,
+      validatorAccount: 'validator address',


So it actually test something:

Suggested change

isVerifiedValidator: false,

validatorAccount: 'validator address',

validatorAccount: 'validator address 2',

thesan · 2023-09-04T15:21:34Z

tests/network-tests/src/flows/membership/updatingProfile.ts

+      isVerifiedValidator: false,
+      validatorAccount: 'validator address',


Then you could try not to pass it to check it remains:

Suggested change

isVerifiedValidator: false,

validatorAccount: 'validator address',

thesan · 2023-09-04T15:22:44Z

tests/network-tests/src/flows/membership/updatingProfile.ts

+      isVerifiedValidator: false,
+      validatorAccount: 'validator address',


Finally based on the mappings it should be removable:

Suggested change

isVerifiedValidator: false,

validatorAccount: 'validator address',

validatorAccount: '',

thesan · 2023-09-04T15:22:55Z

tests/network-tests/src/flows/membership/updatingProfile.ts

+    {
+      handle: 'New handle 1',
+      name: 'New name',
+      isVerifiedValidator: false,


Suggested change

isVerifiedValidator: false,

thesan · 2023-09-04T15:25:45Z

tests/network-tests/src/fixtures/membership/UpdateProfileHappyCaseFixture.ts

+        ? this.newValues.isVerifiedValidator
+        : this.oldValues.isVerifiedValidator,
+      validatorAccount: isSet(this.newValues.validatorAccount)
+        ? this.newValues.validatorAccount


We expect the validatorAccount to be null (i.e removed) when an empty string was passed:

Suggested change

? this.newValues.validatorAccount

? this.newValues.validatorAccount || null

…stream into fix/4823-validator

thesan

Good job just a few last minor changes

thesan · 2023-09-08T12:04:29Z

tests/network-tests/src/Api.ts

@@ -124,7 +124,10 @@ export class ApiFactory {

  constructor(api: ApiPromise, treasuryAccountUri: string, miniSecret: string) {
    this.api = api
-    this.keyring = new Keyring({ type: 'sr25519', ss58Format: JOYSTREAM_ADDRESS_PREFIX })
+    this.keyring = new Keyring({


Please use git to remove the changes on this file to reduce potential conflicts later

tests/network-tests/src/fixtures/membership/utils.ts

tests/network-tests/src/flows/membership/updatingProfile.ts

Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>

thesan

💯

zeeshanakram3

Looks good overall. Please revert the changes in query-node/chain-metadata/*.json files. These are auto-generated files that should be edited.

…stream into fix/4823-validator

zeeshanakram3

LGTM

chrlschwb added 4 commits August 29, 2023 15:00

fix update validator

03c6e81

lint fix

e81b513

update validator

ed17b5d

isVerified is false

6e20a7a

thesan suggested changes Aug 29, 2023

View reviewed changes

chrlschwb added 17 commits August 30, 2023 01:32

git error

321aa4c

fix git error

0d88ad7

TEST update|

da59458

|

pretter on network-test folder

a2eb659

git fix test error

0209d49

fix git

2fbcbd9

add commit for git error fix

27dcd24

test

e675f3a

test

578e849

profo file changed

de56562

membership file change

031eea6

test

bca92db

test

9631880

update membership

6360a23

update graphql file

582f8ca

fix mappings validator of membership

609317c

update test files

d937322

thesan suggested changes Sep 1, 2023

View reviewed changes

chrlschwb and others added 5 commits September 1, 2023 13:11

add test file

e9c4a6e

lint fix error

a1ac5aa

fix error

20c6695

remove .env change

a97377e

test

bf20c3c

thesan suggested changes Sep 4, 2023

View reviewed changes

fix error

7a03c0e

chrlschwb added 14 commits September 7, 2023 04:07

fix error

5cffc23

fix error

38caf6e

lint fix error

75b2502

fix membership test error

962ac30

ont

730e207

test

cd8f4e5

test

eeb9fb1

fix

1afca20

lock file gitignore

bda7607

update

0b49b49

Merge branch 'fix/4823-validator' of https://github.com/chrlschwb/joy…

26eb57e

…stream into fix/4823-validator

fix test error

bdc3c8f

fix test error

b9ee09c

test fix

5f8c523

thesan reviewed Sep 8, 2023

View reviewed changes

chrlschwb and others added 3 commits September 8, 2023 06:04

Update tests/network-tests/src/fixtures/membership/utils.ts

dda8d15

Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>

update

b9a6763

Update tests/network-tests/src/flows/membership/updatingProfile.ts

837f7f1

Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>

thesan approved these changes Sep 11, 2023

View reviewed changes

thesan requested a review from zeeshanakram3 September 11, 2023 07:31

zeeshanakram3 requested changes Sep 11, 2023

View reviewed changes

chrlschwb added 2 commits September 11, 2023 14:33

ignore files

3a2f203

Merge branch 'fix/4823-validator' of https://github.com/chrlschwb/joy…

be58380

…stream into fix/4823-validator

chrlschwb requested a review from zeeshanakram3 September 11, 2023 14:06

zeeshanakram3 approved these changes Sep 11, 2023

View reviewed changes

mnaamani merged commit 8e82ab2 into Joystream:master Sep 13, 2023
23 checks passed

thesan mentioned this pull request Oct 2, 2023

QN: Validator profile scope changes #4905

Closed

2 tasks

thesan added query-node community-dev scope:validator labels Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/4823 validator profile #4849

Fix/4823 validator profile #4849

chrlschwb commented Aug 29, 2023

thesan left a comment

thesan Aug 29, 2023

thesan Aug 29, 2023

chrlschwb Sep 8, 2023

thesan Sep 8, 2023 •

edited

Loading

thesan left a comment

thesan left a comment

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan Sep 4, 2023

thesan left a comment

thesan Sep 8, 2023

thesan left a comment

zeeshanakram3 left a comment

zeeshanakram3 left a comment

		if (typeof metadata?.validatorAccount === 'string') {
		member.metadata.validatorAccount = (metadata.validatorAccount \|\| null) as string

		repeated ExternalResource externalResources = 5;

		optional bool isVerifiedValidator = 6;

	isVerifiedValidator: isSet(this.newValues.isVerifiedValidator)
	? this.newValues.isVerifiedValidator
	: this.oldValues.isVerifiedValidator,

		isVerifiedValidator: false,
		validatorAccount: 'validator address',

	isVerifiedValidator: false,
	validatorAccount: 'validator address',
	validatorAccount: 'validator address 2',

	isVerifiedValidator: false,
	validatorAccount: 'validator address',
	validatorAccount: '',

	? this.newValues.validatorAccount
	? this.newValues.validatorAccount \|\| null

Fix/4823 validator profile #4849

Fix/4823 validator profile #4849

Conversation

chrlschwb commented Aug 29, 2023

thesan 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

thesan Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

thesan left a comment

Choose a reason for hiding this comment

thesan 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

thesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesan left a comment

Choose a reason for hiding this comment

zeeshanakram3 left a comment

Choose a reason for hiding this comment

zeeshanakram3 left a comment

Choose a reason for hiding this comment

thesan Sep 8, 2023 •

edited

Loading