Skip to content
This repository has been archived by the owner on Nov 24, 2022. It is now read-only.

Refactor: CType Utility and Split Metadata and Schema #231

Merged
merged 39 commits into from
Jan 17, 2020
Merged

Refactor: CType Utility and Split Metadata and Schema #231

merged 39 commits into from
Jan 17, 2020

Conversation

Dudleyneedham
Copy link
Member

@Dudleyneedham Dudleyneedham commented Oct 14, 2019 •
edited by tjwelde
Loading

fixes https://github.com/KILTprotocol/ticket/issues/15

Removed CTypeUtils functions from the SDK

  • fromInputModel
  • getClaimInputModel
  • getCTypeInputModel
  • Checked the code in the demo client works

Added them into the demo client.

How to test:

Connect to the chain with the changes, did some test and went through the various Ctype and claims functionality.

You will need to use the Link function and pull this version of the SDK KILTprotocol/sdk-js#211 in order to test.

Then, periodically test the ctype and claims.

Along with the dev tools, though I think they aren't working correctly in the first place.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@Dudleyneedham
Copy link
Member Author

Need to clean up the types used within the functions.

tjwelde
tjwelde suggested changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@tjwelde tjwelde left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall looks pretty good, but I would get rid of the unnecessary functions.
Can we also keep the tests, which are removed here: https://github.com/KILTprotocol/sdk-js/pull/205/files?

src/components/MyClaimCreateView/MyClaimCreateView.tsx Outdated Show resolved Hide resolved
src/containers/CtypeCreate/CtypeCreate.tsx Outdated Show resolved Hide resolved
src/containers/Tasks/SubmitLegitimations/SubmitLegitimations.tsx Outdated Show resolved Hide resolved
src/services/CtypeUtils.ts Outdated Show resolved Hide resolved
@Dudleyneedham
Copy link
Member Author

I have added the test into the file, but they don't work.

@tjwelde
Copy link
Contributor

tjwelde commented Oct 15, 2019 •
edited
Loading

Please put the ctypeutils in the utilsfolder: https://github.com/KILTprotocol/demo-client/tree/dn-demo-200-ctype-utils-refac-objec/src/utils
Also use a subdirectory.

@Dudleyneedham
Copy link
Member Author

I made changes accordingly, additionally.

I was testing the changes in splitting the ctype metadata and schema.

These were closely related and I felt I needed to be added at the same time. They work, but I had to take some checks off. I am trying to think of a way to fix the any.

Dudleyneedham
Dudleyneedham commented Nov 7, 2019
View reviewed changes
src/components/SelectCTypes/SelectCTypes.tsx Outdated
@@ -127,7 +127,7 @@ class SelectCTypes extends React.Component<Props, State> {
return {
baseValue: `${cType.cType.hash}`,
label: <CTypePresentation cTypeHash={cType.cType.hash} />,
value: `${cType.cType.metadata.title.default} ${cType.cType.hash}`,
value: `${cType.cType.schema.$id} ${cType.cType.hash}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

since ctype is of type CTypeMetadata, we should be able to get the title from the metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

@LeonFLK plz check this

tjwelde
tjwelde suggested changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@tjwelde tjwelde left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall. Some nitpicks and a big name refactoring ;)

src/types/Ctype.ts Outdated Show resolved Hide resolved
src/services/DidService.ts Show resolved Hide resolved
src/components/CtypeListView/CtypeListView.tsx Show resolved Hide resolved
src/types/Ctype.ts Outdated Show resolved Hide resolved
src/types/Ctype.ts Outdated Show resolved Hide resolved
src/types/Ctype.ts Outdated Show resolved Hide resolved
src/components/CTypePresentation/CTypePresentation.tsx Outdated Show resolved Hide resolved
src/components/CtypeDetailView/CtypeDetailView.tsx Outdated Show resolved Hide resolved
src/components/SelectCTypes/SelectCTypes.tsx Outdated
@@ -127,7 +127,7 @@ class SelectCTypes extends React.Component<Props, State> {
return {
baseValue: `${cType.cType.hash}`,
label: <CTypePresentation cTypeHash={cType.cType.hash} />,
value: `${cType.cType.metadata.title.default} ${cType.cType.hash}`,
value: `${cType.cType.schema.$id} ${cType.cType.hash}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

since ctype is of type CTypeMetadata, we should be able to get the title from the metadata

src/utils/CtypeUtils/CtypeUtils.ts Outdated Show resolved Hide resolved
tjwelde
tjwelde reviewed Jan 8, 2020
View reviewed changes
src/types/Ctype.ts Outdated Show resolved Hide resolved
@LeonFLK LeonFLK requested a review from tjwelde January 9, 2020 14:34
@tjwelde tjwelde mentioned this pull request Jan 9, 2020
Merged
5 tasks
Dudleyneedham
Dudleyneedham commented Jan 15, 2020
View reviewed changes
src/components/DevTools/DevTools.attestations.tsx

const partialClaim = {
const partialClaim: IPartialClaim = {
cTypeHash: cType.cType.hash as string,
Copy link
Member Author

Choose a reason for hiding this comment

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

Does it need the as string?

Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this ticket / PR

Dudleyneedham
Dudleyneedham commented Jan 15, 2020
View reviewed changes
src/utils/CtypeUtils/CtypeUtils.ts
})
result.required.push(key)
})
Object.entries(ctype.cType.schema.properties as object).forEach(
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we remove the properties as object?

Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this ticket

Dudleyneedham
Dudleyneedham commented Jan 15, 2020
View reviewed changes
src/types/Ctype.ts
@@ -13,8 +15,9 @@ export interface ICTypeInput {
properties: object[] // TO DO: need to refine what properties are
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this should be an Array of objects?

Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this ticket

tjwelde
tjwelde reviewed Jan 15, 2020
View reviewed changes
src/utils/CtypeUtils/CtypeUtils.ts Outdated
result.properties[key].title = getLocalized(value.title, lang)
result.required.push(key)
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this handled in another way now?

tjwelde
tjwelde reviewed Jan 15, 2020
View reviewed changes
Copy link
Contributor

@tjwelde tjwelde left a comment

Choose a reason for hiding this comment

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

Looks very good already, just some small remarks

src/components/SelectCTypes/SelectCTypes.tsx Outdated
@@ -127,7 +127,7 @@ class SelectCTypes extends React.Component<Props, State> {
return {
baseValue: `${cType.cType.hash}`,
label: <CTypePresentation cTypeHash={cType.cType.hash} />,
value: `${cType.cType.metadata.title.default} ${cType.cType.hash}`,
value: `${cType.cType.schema.$id} ${cType.cType.hash}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

@LeonFLK plz check this

src/types/Ctype.ts
@@ -13,8 +15,9 @@ export interface ICTypeInput {
properties: object[] // TO DO: need to refine what properties are
Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this ticket

src/components/DevTools/DevTools.attestations.tsx

const partialClaim = {
const partialClaim: IPartialClaim = {
cTypeHash: cType.cType.hash as string,
Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this ticket / PR

src/utils/CtypeUtils/CtypeUtils.ts
})
result.required.push(key)
})
Object.entries(ctype.cType.schema.properties as object).forEach(
Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this ticket

@LeonFLK
Copy link
Contributor

LeonFLK commented Jan 16, 2020

I looked at the code with Dudley, and we found that getCTypeInputModel does not get called at all, except in the utils test file.

Do we want to remove it?

@tjwelde
Copy link
Contributor

tjwelde commented Jan 17, 2020

Regarding the getCTypeInputModel: We might want to remove it, but not in this PR. Please make a ticket

tjwelde
tjwelde approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@tjwelde tjwelde left a comment

Choose a reason for hiding this comment

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

LGTM

@Dudleyneedham Dudleyneedham merged commit 738a3cc into develop Jan 17, 2020
@Dudleyneedham Dudleyneedham deleted the dn-demo-200-ctype-utils-refac-objec branch January 17, 2020 13:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@LeonFLK LeonFLK LeonFLK left review comments

@tjwelde tjwelde tjwelde approved these changes

@maudnals maudnals Awaiting requested review from maudnals

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dudleyneedham @tjwelde @LeonFLK