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

Download and parse user lists #1699

Merged
merged 13 commits into from
Dec 17, 2024
Merged

Download and parse user lists #1699

merged 13 commits into from
Dec 17, 2024

Conversation

joshuatbrown
Copy link
Contributor

@joshuatbrown joshuatbrown commented Dec 6, 2024

Issues covered

https://github.com/verse-pbc/issues/issues/49

Description

Fetches user lists of kind 30000 and stores them in Core Data when the user's profile is displayed.

  • new Core Data entity: AuthorList
  • EventProcessor learned about kind 30000 events and creates an AuthorList when it sees one
  • the AuthorList isn't displayed anywhere since the ticket doesn't include UI

How to test

Either:
A. Run unit tests
Or:
B. Run the app, find a profile of a user who has lists (npub1yl8jc6znttslcpj3p6p8vuq98awu6w0xh4lqtu0lkjr772kpx4ysfqvz34), then inspect Core Data

To Do

  • Update follow sets when necessary instead of always creating new ones (when a list already exists with the given owner/author + identifier)
  • Do we want AuthorList.create to throw if there's no replaceableID? Probably.
  • Do we want to check the signature before saving the AuthorList? Probably.
  • Decide whether to (a) always fetch all follow sets when loading a profile or (b) track the last time we got them for that profile and only fetch more recent ones (punting for now)
  • Decide what to do in EventProcessor when we get a follow set. Returning nil is weird.
  • Do we want to track deletes like we do for Events? Probably. (punting for now)

In order to track deletes, I think we'll need to look for a plain old Event of kind 5, then follow the same logic we do for finding the Event, but this time, find the AuthorList. We probably need to add a deletedOn to AuthorList to match the one on Event.

In continuing to work through this To Do list, I'm starting to wonder if we should even have a separate AuthorList entity in Core Data. We should certainly have a different model, but I'm not sure about the Core Data entity. There's a lot in common between the new AuthorList and Event in Core Data, and it feels a little weird to have a new entity there when it's just a plain old event like everything else. Unless, of course, we're working towards having separate entities for different types of events.

Copy link
Member

@mplorentz mplorentz left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I also don't like how much code is duplicated between Event and AuthorList. Could you try (perhaps in a future PR?) making AuthorList a child entity and subclass of Event?

@mplorentz
Copy link
Member

Also to address some of your questions in the description:

  • I like lastUpdatedAuthorLists and only requesting newer ones.
  • Maybe the sub-entity thing will help the EventProcessor return value
  • and same for deletion

@joshuatbrown
Copy link
Contributor Author

joshuatbrown commented Dec 10, 2024

Wow, creating a child entity is genius and I'm not sure why I didn't think of it. Did I even know that existed? I can neither confirm nor deny.

I'd especially like to address the EventProcessor return value and handle deletes before merging, so I'll try the child entity in this PR and see how it goes.

@joshuatbrown
Copy link
Contributor Author

Needs re-review since I made AuthorList a child entity and subclass of Event.

@mplorentz
Copy link
Member

👀

Copy link
Member

@mplorentz mplorentz left a comment

Choose a reason for hiding this comment

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

I'm getting an error RelayService: Error parsing events: Could not merge changes.. It looks like some code is creating a kind 30000 event as an Event and not an AuthorList, but I can't find where. Maybe we should just ship the code you had working.

@@ -269,10 +269,27 @@ extension RelayService {
return await fetchEvents(matching: contactFilter)
}

func requestFollowSets(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func requestFollowSets(
func requestAuthorLists(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this specifically requests only kind 30000 Follow Set events, which is why I named it this way. I did consider AuthorLists instead, and I suppose if we want to add more kinds to the Filter in the future, we'd want that to be the name. I guess for now this does fetch all AuthorLists we support, which happens to be only kind 30000 Follow Sets, so I'm good with the rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed!

Copy link
Contributor

@bryanmontz bryanmontz left a comment

Choose a reason for hiding this comment

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

It looks good, and I can't reproduce the event parsing errors you have shown. I would remove the extraneous "lists" relationship.

Nos/Service/EventProcessor.swift Outdated Show resolved Hide resolved
<relationship name="follows" optional="YES" toMany="YES" deletionRule="Cascade" destinationEntity="Follow" inverseName="source" inverseEntity="Follow"/>
<relationship name="includedInLists" optional="YES" toMany="YES" deletionRule="Nullify" destinationEntity="AuthorList" inverseName="authors" inverseEntity="AuthorList"/>
<relationship name="incomingNotifications" optional="YES" maxCount="1" deletionRule="Nullify" destinationEntity="NosNotification" inverseName="user" inverseEntity="NosNotification"/>
<relationship name="lists" optional="YES" toMany="YES" deletionRule="Nullify" destinationEntity="AuthorList"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Xcode is unhappy that this "lists" relationship has no inverse. It seems the same as "includedInLists" though. Did you mean to have both of them?

Nos_—_Nos_21_xcdatamodel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An Author can have multiple AuthorLists that they own. In each AuthorList is an array of Authors. So there are two relationships here, I think, and I suppose I need to handle that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I see now that if I set the inverse of lists to author, then events no longer has the inverse set to author. So that's a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and added another relationship to AuthorList: owner, which goes to Author, and allows us to set the Author inverse to owner.

# Conflicts:
#	CHANGELOG.md
#	Nos.xcodeproj/project.pbxproj
@joshuatbrown joshuatbrown added this pull request to the merge queue Dec 17, 2024
Merged via the queue into main with commit 6284443 Dec 17, 2024
4 checks passed
@joshuatbrown joshuatbrown deleted the lists branch December 17, 2024 19:12
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.

3 participants