the-blue-alliance / the-blue-alliance-ios

An iOS app for accessing information about the FIRST Robotics Competition.
MIT License
76 stars 23 forks source link

Teams are not sorted until pulling down to refresh #525

Closed michaelhill closed 5 years ago

michaelhill commented 5 years ago

When selecting the competition (in this particular instance, 2019 Daly Championship Division), but I saw it with other divisions, the teams are not sorted by rank until pulled down (to refresh).

Expected Behavior

When I tap on the rankings tab, all teams should be sorted by ranking

Current Behavior

After not using the app for some time (in my case, it was several hours), I tapped on a competition name, In this case, 2019 Daly Championship Division, and saw the teams were not sorted by rank. I can't remember if I had accessed the app after matches had ended the day prior to noticing this bug. After pulling down to refresh, the teams were sorted. My conjecture is that it pulled updated data from the TBA API when I opened up the competition, but did not perform a sort operation.

Screenshots

IMG_0594

Steps to Reproduce

  1. During a competition day (when rankings are changing), use the app to view different active competitions (and probably view rankings at some point)
  2. Wait a few hours
  3. Open up the app back up and access the same competitions accessed in step 1. (this can be after competition is done for the day)
  4. Tap on the rankings (At this point, they will probably be unsorted, but wins/losses/ranking appear to be complete and correct)
  5. Pull down to refresh (teams are now sorted by ranking).

Your Environment

Context

I am just a user wanting to know how a few teams I am interested in are doing and to see how the rankings have panned out at the championship competitions.

Possible Solution

I don't know anything about iOS programming, but my gut feeling is that while data is pulled from the TBA API when clicking on the ranking tab (or at some point prior), sorting by ranking isn't accomplished by this operation. Perhaps there's some race condition? Does sorting happen when the ranking tab is tapped, but doesn't have the data from the API yet?

ZachOrr commented 5 years ago

Marking this one as high priority - we've seen lots of reports about this.

kevinbai0 commented 5 years ago

Not too sure how to test right now since there's no event (maybe at the next offseason event) but I'm guessing that tableView.reloadData() at viewDidAppear should solve this in EventRankingsViewController.

ZachOrr commented 5 years ago

Mmm throwing a reload in here doesn't seem like the right approach. We need to figure out the root cause of this one.

kevinbai0 commented 5 years ago

Alright, I think I found a potential scenario that would cause this issue. A user refreshes in EventInfoViewController which then calls refresh and which tbaKit.fetchEvent(...) in EventInfoViewController. This updates the event model and only reloads the table in that VC. Then, the user immediately switches to EventRankingViewController. shouldRefresh returns false since the user has already refreshed the rankings earlier in the day, and the automatic refresh interval hasn't been hit yet. Therefore, refresh isn't called, which means that tableView.reloadData isn't called either, the event data itself is up to date since it was refreshed in EventInfoViewController, but may not be sorted in the hierarchy. This scenario would cause that issue (assuming that I didn't miss something).

ZachOrr commented 5 years ago

EventInfoViewController and EventRankingsViewController both have their own refresh keys, which mean they have their own refresh intervals. Refreshing info won't lock out automatically refresh rankings.

The view should be initially loaded in sorted order when navigating to a EventRankingsViewController because of our sort descriptors on our NSFetchRequest tied to that table view.

This feels like a bug somewhere in our Core Data stack, where Core Data is returning us previously cached results for our fetch request without re-sorting them. I have no idea why that would be happening, since I'm fairly confident we don't cache our Core Data results anywhere. We might look in to what Core Data's default caching strategy looks like, and if anyone has run in to this more broadly.

kevinbai0 commented 5 years ago

Right, but a few things I've noticed: It only performs a fetch request at initialization or at reconfigureFetchRequest. However, reconfigureFetchRequest is never called (updateDataSource). It also means that deleteCache in EventRankingsVC is never called after the app is initialized (app could be running in the background for a long time) - this could be the caching issue.

Does NSFetchRequest only resort the data on a refresh? If so, the scenario above would still be entirely possible since the automatic refresh interval is one hour (even though one refresh won't lock out another, the other still might have 20 minutes to go before another refresh is up).

ZachOrr commented 5 years ago

We only need to setup a fetch request once for our NSFetchedResultsController. Afterwards, our FRC keeps our table view in sync by dispatching a reload in controllerDidChangeContent. I'm actually fairly certain we don't need that updateDataSource method in our rankings VC, since it's unused. But usually it's used if your underlying data needs to be updated to match a different fetch request (see WeekEventsViewController/EventsViewController, when we change our weekEvent)

My thought is, since we're loading a new EventRankingsVC when we push to our EventVC (assuming there's no retain cycle somewhere) our initialization is always called. We'll always set up a new FR/FRC, we'll always kickoff an initial reload of the table view. Objects are being passed to our configure method, they're just in the wrong order.

ZachOrr commented 5 years ago

I'm going to close this one, since I can't reproduce it. We can keep an eye on it moving forward, since multiple people have reported this. But we were unable to replicate it in testing.