Skip to content

session4_data_models_p2#9

Open
Joshfairhead wants to merge 1 commit intoholochain-devcamp:session4_data_models_p2from
Joshfairhead:patch-1
Open

session4_data_models_p2#9
Joshfairhead wants to merge 1 commit intoholochain-devcamp:session4_data_models_p2from
Joshfairhead:patch-1

Conversation

@Joshfairhead
Copy link

Wow I'm struggling with git lol, cant seem to pull the various branches with homework to local, just master... so making this PR is a total hack via github branch!

I've had the advantage of being able to see the answers this time round which made implementing the CRUDs easier... but I did not copy them directly! My strategy was to give it a go by directly copying the bits that were already implemented and changing the names (which is how I did it last time)... that turned out to be wrong when I checked it against the solution. What I found was that the pieces to feed into the function (are these methods?) was that the fields I needed were either in the module of entry/anchor or in the handlers... I'm not sure where I should be lifting the fields from to be honest or how they connect yet despite watching the videos a bunch of times... so behind on validation at this point...

Wow I'm struggling with git lol, cant seem to pull the various branches with homework to local, just master... so making this PR is a total hack via github branch!

I've had the advantage of being able to see the answers this time round which made implementing the CRUDs easier... but I did not copy them directly! My strategy was to give it a go by directly copying the bits that were already implemented and changing the names (which is how I did it last time)... that turned out to be wrong when I checked it against the solution. What I found was that the pieces to feed into the function (are these methods?) was that the fields I needed were either in the module of entry/anchor or in the handlers... I'm not sure where I should be lifting the fields from to be honest or how they connect yet despite watching the videos a bunch of times... so behind on validation at this point...
}
}

// sections_address - do I need to replicate this in the sections CRUD? Seems weird to update the course this way.

Choose a reason for hiding this comment

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

hi josh! Yep, we would need to have an update_section function as well.
update_course updates the entire course entry. This also makes it possible for update_course to technically either add or remove section in the sections field of course struct. However, if we do want to update the details of the section itself (which for now is only title), then we would need the update_section function :D

Comment on lines +132 to +139
fn update_section(
title: String,
course_anchor_address: Address,
timestamp: u64,
anchor_address: Address,
) -> ZomeApiResult<Address> {
section::handlers::update(title, course_anchor_address, timestamp, anchor_address)
}

Choose a reason for hiding this comment

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

hi josh! For this one, we will need the section_anchor_address only and not the course_anchor_address! We don't need to change anything in the course since course entry stores the anchor address of the section. This means that no matter how many times we update the section entry, the section_anchor_address stored in the course entry will always point to the latest version of the section entry!
With this my suggestion would be,

Suggested change
fn update_section(
title: String,
course_anchor_address: Address,
timestamp: u64,
anchor_address: Address,
) -> ZomeApiResult<Address> {
section::handlers::update(title, course_anchor_address, timestamp, anchor_address)
}
fn update_section(
title: String,
section_anchor_address: Address,
timestamp: u64,
) -> ZomeApiResult<Address> {
section::handlers::update(title, section_anchor_address, timestamp)
}

Comment on lines +142 to +144
fn delete_section(section_anchor_address: Address) -> ZomeApiResult<Address> {
section::handlers::delete(section_anchor_address)
}

Choose a reason for hiding this comment

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

With the explanation nastasia gave us last session, we would need to add timestamp in delete_section and update the timestamp of the course entry so that we will not be ending up having duplicate course entries (which causes the bug nastasia mentioned last session :D)

Suggested change
fn delete_section(section_anchor_address: Address) -> ZomeApiResult<Address> {
section::handlers::delete(section_anchor_address)
}
fn delete_section(section_anchor_address: Address, timestamp: u64) -> ZomeApiResult<Address> {
section::handlers::delete(section_anchor_address, timestamp)
}

Comment on lines +171 to +179
fn update_content(
name: String,
section_anchor_address: Address,
url: String,
timestamp: u64,
description: String,
) -> ZomeApiResult<Address> {
content::handlers::update(name, url, description, timestamp, section_anchor_address,)
}

Choose a reason for hiding this comment

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

In order for us to know which content we would like to update, we would need to provide the content_address of the content we are trying to update. And since content entry already has the section_anchor_address, we wouldnt be needing the section_anchor_address as the parameter of this function.

Suggested change
fn update_content(
name: String,
section_anchor_address: Address,
url: String,
timestamp: u64,
description: String,
) -> ZomeApiResult<Address> {
content::handlers::update(name, url, description, timestamp, section_anchor_address,)
}
fn update_content(
name: String,
content_address: Address,
url: String,
timestamp: u64,
description: String,
) -> ZomeApiResult<Address> {
content::handlers::update(name, url, description, timestamp, content_address)
}

Copy link

@tatssato tatssato left a comment

Choose a reason for hiding this comment

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

Hi josh! Tats here.

I've had the advantage of being able to see the answers this time round which made implementing the CRUDs easier... but I did not copy them directly!

This is great to know!

What I found was that the pieces to feed into the function (are these methods?) was that the fields I needed were either in the module of entry/anchor or in the handlers... I'm not sure where I should be lifting the fields from to be honest or how they connect yet despite watching the videos a bunch of times... so behind on validation at this point...

So the functions you find in this lib.rs is the top level zome functions that will be exposed outside of conductor (frontend, terminal, tryorama test, etc). And each of the function has a handler which we need to pass the correct arguments into in order for us to implement the necessary CRUD operations on section and content. We really need to understand the design of LeaP (see the data model nastasia made) and also think through of what can be found from the entry themselves and what data we need from the caller of these zome functions. So for example, if we want to update a content, we would need the input of the caller of this function when it comes to name, url, timestamp, description and lastly content_address to know which content we would like to update. However, in updating the content, we need to remove the link of the previous content to the section anchor and link the updated content to the section anchor. This requires us to know the section_anchor_address as well. However, we do not need to add section_anchor_address into the parameter of the top level zome function since section_anchor_address is part of the content struct and we can get it using the content_address we specified as a parameter!

I hope this helps!

I also made some suggestions to your PRs so you could check them out as well :D

@Joshfairhead
Copy link
Author

Thanks for all the feedback Tats, much appreciate the code review. Getting there slowly which is good, though I'm not sure what you mean about the top level zome calls to update content -> section_anchor_address via the content struct? Is the structs contents exposed to the lib.rs and thus more granular functionality just doesn't need to be stipulated at the top level?

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