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

talk page - CoreData housekeeping #3134

Merged
merged 5 commits into from Jun 18, 2019
Merged

talk page - CoreData housekeeping #3134

merged 5 commits into from Jun 18, 2019

Conversation

tonisevener
Copy link
Collaborator

Delete talk pages > 50

Delete talk pages > 50
@@ -124,7 +135,7 @@ extension NSManagedObjectContext {
func removeUnlinkedTalkPageTopicContent() throws {
let request: NSFetchRequest<TalkPageTopicContent> = TalkPageTopicContent.fetchRequest()
request.predicate = NSPredicate(format: "topics.@count == 0")
let results = try request.execute()
let results = try fetch(request)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

before this change I was getting a "Cannot fetch without an NSManagedObjectContext in scope}" console error and TalkPageTopicContent objects were not clearing out in housekeeping.

@@ -70,6 +70,7 @@
</fetchIndex>
</entity>
<entity name="TalkPage" representedClassName="TalkPage" syncable="YES" codeGenerationType="class">
<attribute name="dateAccessed" attributeType="Date" defaultDateTimeInterval="582233760" usesScalarValueType="NO" syncable="YES"/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This defaults to the date/time I created the attribute (06/14/2019), let me know if there's a better one.


let oldTalkPages = try moc.fetch(request)
for talkPage in oldTalkPages {
moc.delete(talkPage)
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -20,14 +20,23 @@ extension NSManagedObjectContext {
fetchRequest.fetchLimit = 1
fetchRequest.predicate = NSPredicate(format: "key == %@", databaseKey)

return try fetch(fetchRequest).first
let talkPage = try fetch(fetchRequest).first
talkPage?.dateAccessed = Date()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned that the method for fetching a talk page also mutates it and saves the context - could I move this into a separate function that's explicitly called when the user views the talk page or is there some other consideration that I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I misread "accessed" to mean "viewed by the user" - is the intent to have this property updated anytime the object is accessed even by non-user task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joewalsh true, agreed, non-user task shouldn't update this in my opinion. I really meant viewed by the user & wanted to keep track of what talk pages they seem to be interested in lately and update that date. Then later I sort by that date so we are deleting talk pages that they don't seem to be visiting anymore. To keep it simpler we could probably only timestamp this after it's fetched in the view controller so we know it was instigated by the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tonisevener makes sense. I pushed a change, but feel free to alter or revert if you had other ideas

@tonisevener
Copy link
Collaborator Author

tonisevener commented Jun 17, 2019

Updated with PR feedback -

  1. I removed dateAccessed timestamp from TalkPageLocalHandler, I think only setting it in TalkPageContainerVC should be enough.
  2. Added batch delete in deleteStaleTalkPages & removeUnlinkedTalkPageTopicContent and am merging the changes in case the user has that talk page in memory. I didn't bother with reacting on the UI-side to a talk page deleting. I don't really see how a talk page currently on screen could become the 50th talk page sorted by access date and delete out from under them in housekeeping, so it seems unnecessary. But, I'm happy to handle if you all think otherwise.

@joewalsh joewalsh merged commit 57a4fe5 into develop Jun 18, 2019
@joewalsh joewalsh deleted the bug/housekeeping branch June 18, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants