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

Initial talk page work #3066

Merged
merged 158 commits into from May 31, 2019
Merged

Initial talk page work #3066

merged 158 commits into from May 31, 2019

Conversation

tonisevener
Copy link
Collaborator

No description provided.

@tonisevener tonisevener added this to the 6.3 milestone Apr 29, 2019
@tonisevener
Copy link
Collaborator Author

Not much to see on the UI side of things but please let me know if I'm off track with any of this so far. I will be switching to integrating the talk page endpoint & list screen UI now.

@tonisevener tonisevener changed the title Initial talk page list work Initial talk page work Apr 29, 2019
</entity>
<entity name="TalkPageDiscussionItem" representedClassName="TalkPageDiscussionItem" syncable="YES" codeGenerationType="class">
<attribute name="depth" attributeType="Integer 16" defaultValueString="0" usesScalarValueType="YES" syncable="YES"/>
<attribute name="text" attributeType="String" syncable="YES"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also keep the text in a file and store a reference as an attribute on TalkPageDiscussionItem, in some cases there could be a lot of text

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good to know, I was unsure if it would be overkill so I didn't want to tackle it initially - I'll look into that after I flesh out the UI more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI planning to do this separately as a part of https://phabricator.wikimedia.org/T222721.

}

//first delete old discussions
//todo: is there a better way to do this? simply reassigning discussions is not enough
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding this correctly, you could compare your existing discussions with the discussions that are coming from the server and figure out what changed (what was deleted, what was added, what was updated) and then process those specific changes - we do something like that for reading lists -

func executeFullSync(on moc: NSManagedObjectContext) throws {

specifically here's where the updates happen -

syncedReadingListEntriesCount += try createOrUpdate(remoteReadingListEntries: remoteReadingListEntries, for: readingListID, deleteMissingLocalEntries: true, inManagedObjectContext: moc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah that would be better. Right now I'm just throwing out all discussions and recreating them if the revision ID has changed. The question was more around delete rules - the old discussions still hang around in the DB even after I reassign the talk page to a new set of discussions. This was a note to look into a way to have that delete cascade on that assignment without explicitly looping through and deleting them all. But agreed being more selective about what I delete would be a better way to go.

@tonisevener tonisevener removed the WIP label May 31, 2019
@tonisevener
Copy link
Collaborator Author

Not feature complete but I think this is far enough along to review for merging into develop.

@tonisevener tonisevener merged commit a35ded8 into develop May 31, 2019
@tonisevener tonisevener deleted the feature/T215928-initial branch May 31, 2019 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants