Question: Deleting assignments rows on sendProgress?

[Deadpikle]

Howdy,

Currently the app has an issue where the time between finishing reviews (of any count) and finishing syncing data, the home screen info on assignments, recent lessons, etc. will potentially be inaccurate.

I believe this is because the assignments data is deleted in sendProgress, thus until the assignments data is re-synced from WaniKani, any pulls of data involving the assignments table in the local db will be inaccurate.

tsurukame/ios/LocalCachingClient.swift

Lines 735 to 743 in a1ffaa0

	func sendProgress(_ progress: [TKMProgress]) -> Promise<Void> {
	db.inTransaction { db in
	for p in progress {
	// Delete the assignment.
	db.mustExecuteUpdate("DELETE FROM assignments WHERE id = ?", args: [p.assignment.id])
	// Store the progress locally.
	db.mustExecuteUpdate("REPLACE INTO pending_progress (id, pb) VALUES (?, ?)",
	args: [p.assignment.subjectID, try! p.serializedData()])

This explains why sometimes after finishing reviews my current vs next level graphs will be off/the wrong levels, or the new Recent Lessons number will be off as the JOIN from subject_progress to assignments doesn't join for any just-finished reviews (the assignments portion is NULL on JOIN), etc.

Is there a reason why sendProgress couldn't update the pertinent assignments data directly and then a new sync from WaniKani would do a full update as well with any pertinent info from the API or similar? This would prevent the few seconds of UI desync, although I'm not sure how the problem would be affected by a spotty or offline internet connection.

Looks like the given line has been around since 043448f (Dec 2017).

[davidsansome]

Is there a reason why sendProgress couldn't update the pertinent assignments data directly and then a new sync from WaniKani would do a full update as well with any pertinent info from the API or similar?

Yeah I think that could probably work. It would need to clear the hasAvailableAt field in the assignment's proto data so it's not considered for reviews/lessons any more.

I think it was probably done this way because all the home screen stats were added later and we never came back to see if there was a smarter way to deal with the reviews that had been done but not sync'd yet.

[Deadpikle]

Makes sense. Thanks.

A quick smoke test of the following code:

      for p in progress {
        // Set the assignment as not available.
        var assignment = p.assignment
        assignment.clearAvailableAt()
        db.mustExecuteUpdate("UPDATE assignments SET pb = ? WHERE id = ?",
                             args: [try! assignment.serializedData(), assignment.id])

doesn't exhibit any obvious bugs, but I didn't do a lot of testing with things like dropped internet connections, staying offline, etc.

EDIT: This would exhibit bugs in the following:

• User makes a mistake (+1 recent mistake)
• User finishes reviews
• Main menu shows 1 recent mistake
• User wants to do recent mistake reviews before app syncs (see TestFlight crash when opening recent mistakes review #688 for bug on this)
• App does not recognize this recent mistake as an actual thing to review because hasAvailableAt is now false, so the recent mistake count was actually wrong (and/or otherwise not accurate). The query that gets that count needs to check hasAvailableAt or we would need another fix to make this all work nicely.

[davidsansome]

I've committed a quick fix for the crash, but the underlying issue still remains.