Githawk: Mark read on open is triggered too soon

[Sherlouk]

If the request fails for whatever reason it’s possible that it gets marked as read but the issue doesn’t load (just experienced this on the train with intermittent network)

Bug Report Dump (Auto-generated)

Version 1.11.0 (1942)
Device: iPhone 6s Plus (iOS 11.1)
TestFlight: true

[rnystrom]

Hmm good find. Gonna low pri this since it wouldn’t irk me too much if this happened. Probably have to rearrange where we kick off the read request.

_{Sent with GitHawk}

[BasThomas]

We should do it after the issue loads then? If it fails at that point, it also wouldn’t be in sync (obviously), but I think that scenario is better than this one.

Also has me thinking about the ability to reload / force refresh an issue if it fails to load entirely.

_{Sent with GitHawk}

[Sherlouk]

I’d always rather it fail to mark as read if I can see an issue rather than it successfully mark as read but I haven’t even seen it

I get low pri but honestly I use read/unread as a reminder of what to respond to - so if it marks as read and I forget to read it because it failed then Yano not good!

Agree we just need to move the call to the load completion shouldn’t be a big change!

_{Sent with GitHawk}

[vadimue]

Hi guys. I want to contribute to this repo and start from this issue. Can you help me? The project has complex structure, which module/ViewController need to update?

[Sherlouk]

@vadimue Sure! This should be a fairly easy task for you to pick up!

So all of the Inbox/Notifications items will be inside of the Notifications group. We use "section controllers" to handle the logic for a given a section and that's where we create the cells themselves. For this issue in particular we want to look at the NotificationSectionController. I've highlighted in that link the block of code which currently marks it at read (causing this bug). This is triggered on didSelect irrelevant of whether the actual issue was shown.

As you'll see a little bit further down that same function we create an IssuesViewController in the eventuality that the notification is for an issue or pull request. Within the IssuesViewController we have a variable called current. This is what stores a reference to an issue once we've fetched it's content from the network (and therefore can display it) - That is where the mark as read logic should be moved to.

We also need to add logic for commit notifications (The only other possibility if it's not an issue/pull request). This one is more difficult because it just opens a SafariViewController of which we currently have no delegate to find out when it's actually loaded. For this I'd be happy to just keep the logic as it was before, in didSelectItem (but obviously just for commits, so it'll need moving down a bit). Transversely, we could do a slightly bigger change to add the ability to add a completion block to the presentCommit function - but I'll leave that up to you!

[vadimue]

@Sherlouk Thanks for the answer! Maybe it is more simple to create a delegate or closure for notifying NotificationSectionController when an issue was downloaded? Because at the IsuesViewController I have to make the request (NotificationClient is absent on this VC) and update cell from the previous screen.

[Sherlouk]

If that's the way you feel it should be done then feel free to try it! 😄

I'm not sure the reason why mark as read was wrapped in it's own container, maybe something to do with the handlers? Ultimately the request can be triggered from the GitHub client (as it does just bridge it anyway), so it may be possible to move the markAsRead to the GithubClient, in the NotificationClient just call that new function, and then from the IssuesViewController you have everything you need

Like I said though, do what you feel comfortable with!

[rnystrom]

Closing since hasn't been reported or brought up in ages