From f60f1d861063e02a52fa9731f7f3a958be77ce7d Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Fri, 3 May 2019 17:11:28 +0100 Subject: [PATCH 01/10] Fix store name --- .../Stats/Insights/SiteStatsInsightsViewModel.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsViewModel.swift b/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsViewModel.swift index dd28cd0c4608..eded3a98df05 100644 --- a/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsViewModel.swift +++ b/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsViewModel.swift @@ -51,8 +51,8 @@ class SiteStatsInsightsViewModel: Observable { var tableRows = [ImmuTableRow]() - if store.fetchingOverviewHasFailed && - !store.containsCachedData { + if insightsStore.fetchingOverviewHasFailed && + !insightsStore.containsCachedData { return ImmuTable(sections: []) } From 12da33dd027d8edaea37f506105304985f378f14 Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Fri, 3 May 2019 17:11:49 +0100 Subject: [PATCH 02/10] Move code in an extension --- ...SiteStatsInsightsTableViewController.swift | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsTableViewController.swift b/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsTableViewController.swift index b066ab454052..540d2e360653 100644 --- a/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsTableViewController.swift +++ b/WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsTableViewController.swift @@ -40,7 +40,7 @@ enum InsightType: Int { @objc optional func showPostStats(postID: Int, postTitle: String?, postURL: URL?) } -class SiteStatsInsightsTableViewController: UITableViewController, NoResultsViewHost { +class SiteStatsInsightsTableViewController: UITableViewController { // MARK: - Properties @@ -121,32 +121,6 @@ private extension SiteStatsInsightsTableViewController { TableFooterRow.self] } - func displayLoadingViewIfNecessary() { - guard tableHandler.viewModel.sections.isEmpty else { - return - } - - configureAndDisplayNoResults(on: tableView, - title: NoResultConstants.successTitle, - accessoryView: NoResultsViewController.loadingAccessoryView()) { [weak self] noResults in - noResults.delegate = self - noResults.hideImageView(false) - } - } - - func displayFailureViewIfNecessary() { - guard tableHandler.viewModel.sections.isEmpty else { - return - } - - updateNoResults(title: NoResultConstants.errorTitle, - subtitle: NoResultConstants.errorSubtitle, - buttonTitle: NoResultConstants.refreshButtonTitle) { [weak self] noResults in - noResults.delegate = self - noResults.hideImageView() - } - } - // MARK: - Table Refreshing func refreshTableView() { @@ -158,8 +132,8 @@ private extension SiteStatsInsightsTableViewController { tableHandler.viewModel = viewModel.tableViewModel() - if store.fetchingOverviewHasFailed && - !store.containsCachedData { + if insightsStore.fetchingOverviewHasFailed && + !insightsStore.containsCachedData { displayFailureViewIfNecessary() } else { hideNoResults() @@ -208,8 +182,36 @@ private extension SiteStatsInsightsTableViewController { let insightTypesInt = insightsToShow.compactMap { $0.rawValue } UserDefaults.standard.set(insightTypesInt, forKey: userDefaultsKey) } +} + +extension SiteStatsInsightsTableViewController: NoResultsViewHost { + private func displayLoadingViewIfNecessary() { + guard tableHandler.viewModel.sections.isEmpty else { + return + } + + configureAndDisplayNoResults(on: tableView, + title: NoResultConstants.successTitle, + accessoryView: NoResultsViewController.loadingAccessoryView()) { [weak self] noResults in + noResults.delegate = self + noResults.hideImageView(false) + } + } + + private func displayFailureViewIfNecessary() { + guard tableHandler.viewModel.sections.isEmpty else { + return + } + + updateNoResults(title: NoResultConstants.errorTitle, + subtitle: NoResultConstants.errorSubtitle, + buttonTitle: NoResultConstants.refreshButtonTitle) { [weak self] noResults in + noResults.delegate = self + noResults.hideImageView() + } + } - enum NoResultConstants { + private enum NoResultConstants { static let successTitle = NSLocalizedString("Loading Stats...", comment: "The loading view title displayed while the service is loading") static let errorTitle = NSLocalizedString("Stats not loaded", comment: "The loading view title displayed when an error occurred") static let errorSubtitle = NSLocalizedString("There was a problem loading your data, refresh your page to try again.", comment: "The loading view subtitle displayed when an error occurred") From af3490a50ecf87ce1f6b574d2ba287a3539f4a54 Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Fri, 3 May 2019 17:13:28 +0100 Subject: [PATCH 03/10] Implement loading view --- .../SiteStatsPeriodTableViewController.swift | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift index adaebc43ebee..e30b29b89b92 100644 --- a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift +++ b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift @@ -46,6 +46,8 @@ class SiteStatsPeriodTableViewController: UITableViewController { } else { refreshData() } + + displayLoadingViewIfNecessary() } } @@ -137,6 +139,8 @@ private extension SiteStatsPeriodTableViewController { tableHandler.viewModel = viewModel.tableViewModel() refreshControl?.endRefreshing() + hideNoResults() + // Scroll to the top of the table. // TODO: look at removing this when loading view is added. tableView.scrollToRow(at: IndexPath(row: 0, section: 0), at: .top, animated: true) @@ -179,6 +183,55 @@ private extension SiteStatsPeriodTableViewController { } +// MARK: - NoResultsViewHost + +extension SiteStatsPeriodTableViewController: NoResultsViewHost { + private func displayLoadingViewIfNecessary() { + guard tableHandler.viewModel.sections.isEmpty else { + return + } + + configureAndDisplayNoResults(on: tableView, + title: NoResultConstants.successTitle, + accessoryView: NoResultsViewController.loadingAccessoryView()) { [weak self] noResults in + noResults.delegate = self + noResults.hideImageView(false) + } + } + + private func displayFailureViewIfNecessary() { + guard tableHandler.viewModel.sections.isEmpty else { + return + } + + updateNoResults(title: NoResultConstants.errorTitle, + subtitle: NoResultConstants.errorSubtitle, + buttonTitle: NoResultConstants.refreshButtonTitle) { [weak self] noResults in + noResults.delegate = self + noResults.hideImageView() + } + } + + private enum NoResultConstants { + static let successTitle = NSLocalizedString("Loading Stats...", comment: "The loading view title displayed while the service is loading") + static let errorTitle = NSLocalizedString("Stats not loaded", comment: "The loading view title displayed when an error occurred") + static let errorSubtitle = NSLocalizedString("There was a problem loading your data, refresh your page to try again.", comment: "The loading view subtitle displayed when an error occurred") + static let refreshButtonTitle = NSLocalizedString("Refresh", comment: "The loading view button title displayed when an error occurred") + } +} + +// MARK: - NoResultsViewControllerDelegate methods + +extension SiteStatsPeriodTableViewController: NoResultsViewControllerDelegate { + func actionButtonPressed() { + updateNoResults(title: NoResultConstants.successTitle, + accessoryView: NoResultsViewController.loadingAccessoryView()) { noResults in + noResults.hideImageView(false) + } + refreshData() + } +} + // MARK: - SiteStatsPeriodDelegate Methods extension SiteStatsPeriodTableViewController: SiteStatsPeriodDelegate { From 0f580ec15bc94ddc339be543408a323968028754 Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Tue, 7 May 2019 13:44:18 +0100 Subject: [PATCH 04/10] Fix add subview if animated var is false --- WordPress/Classes/Extensions/UIViewController+NoResults.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/WordPress/Classes/Extensions/UIViewController+NoResults.swift b/WordPress/Classes/Extensions/UIViewController+NoResults.swift index fd8461be5a88..7744a8e49150 100644 --- a/WordPress/Classes/Extensions/UIViewController+NoResults.swift +++ b/WordPress/Classes/Extensions/UIViewController+NoResults.swift @@ -116,6 +116,8 @@ extension NoResultsViewHost where Self: UIViewController { if animated { view.addSubview(withFadeAnimation: noResultsViewController.view) + } else { + view.addSubview(noResultsViewController.view) } noResultsViewController.didMove(toParent: self) } From e81c9bf868435dffd3d61b2acb2117126f4589ef Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Tue, 14 May 2019 15:28:04 +0100 Subject: [PATCH 05/10] Fix store to handle the errors --- .../Classes/Stores/StatsPeriodStore.swift | 152 ++++++++++++------ 1 file changed, 99 insertions(+), 53 deletions(-) diff --git a/WordPress/Classes/Stores/StatsPeriodStore.swift b/WordPress/Classes/Stores/StatsPeriodStore.swift index 7251d25a2dc8..6d4baa11a27b 100644 --- a/WordPress/Classes/Stores/StatsPeriodStore.swift +++ b/WordPress/Classes/Stores/StatsPeriodStore.swift @@ -5,15 +5,15 @@ import WordPressComStatsiOS enum PeriodAction: Action { // Period overview - case receivedSummary(_ summary: StatsSummaryTimeIntervalData?) - case receivedPostsAndPages(_ postsAndPages: StatsTopPostsTimeIntervalData?) - case receivedPublished(_ published: StatsPublishedPostsTimeIntervalData?) - case receivedReferrers(_ referrers: StatsTopReferrersTimeIntervalData?) - case receivedClicks(_ clicks: StatsTopClicksTimeIntervalData?) - case receivedAuthors(_ authors: StatsTopAuthorsTimeIntervalData?) - case receivedSearchTerms(_ searchTerms: StatsSearchTermTimeIntervalData?) - case receivedVideos(_ videos: StatsTopVideosTimeIntervalData?) - case receivedCountries(_ countries: StatsTopCountryTimeIntervalData?) + case receivedSummary(_ summary: StatsSummaryTimeIntervalData?, _ error: Error?) + case receivedPostsAndPages(_ postsAndPages: StatsTopPostsTimeIntervalData?, _ error: Error?) + case receivedPublished(_ published: StatsPublishedPostsTimeIntervalData?, _ error: Error?) + case receivedReferrers(_ referrers: StatsTopReferrersTimeIntervalData?, _ error: Error?) + case receivedClicks(_ clicks: StatsTopClicksTimeIntervalData?, _ error: Error?) + case receivedAuthors(_ authors: StatsTopAuthorsTimeIntervalData?, _ error: Error?) + case receivedSearchTerms(_ searchTerms: StatsSearchTermTimeIntervalData?, _ error: Error?) + case receivedVideos(_ videos: StatsTopVideosTimeIntervalData?, _ error: Error?) + case receivedCountries(_ countries: StatsTopCountryTimeIntervalData?, _ error: Error?) case refreshPeriodOverviewData(date: Date, period: StatsPeriodUnit, forceRefresh: Bool) // Period details @@ -109,30 +109,39 @@ struct PeriodStoreState { var summary: StatsSummaryTimeIntervalData? var fetchingSummary = false + var fetchingSummaryHasFailed = false var topPostsAndPages: StatsTopPostsTimeIntervalData? var fetchingPostsAndPages = false + var fetchingPostsAndPagesHasFailed = false var topReferrers: StatsTopReferrersTimeIntervalData? var fetchingReferrers = false + var fetchingReferrersHasFailed = false var topClicks: StatsTopClicksTimeIntervalData? var fetchingClicks = false + var fetchingClicksHasFailed = false var topPublished: StatsPublishedPostsTimeIntervalData? var fetchingPublished = false + var fetchingPublishedHasFailed = false var topAuthors: StatsTopAuthorsTimeIntervalData? var fetchingAuthors = false + var fetchingAuthorsHasFailed = false var topSearchTerms: StatsSearchTermTimeIntervalData? var fetchingSearchTerms = false + var fetchingSearchTermsHasFailed = false var topCountries: StatsTopCountryTimeIntervalData? var fetchingCountries = false + var fetchingCountriesHasFailed = false var topVideos: StatsTopVideosTimeIntervalData? var fetchingVideos = false + var fetchingVideosHasFailed = false // Post Stats @@ -155,24 +164,24 @@ class StatsPeriodStore: QueryStore { } switch periodAction { - case .receivedSummary(let summary): - receivedSummary(summary) - case .receivedPostsAndPages(let postsAndPages): - receivedPostsAndPages(postsAndPages) - case .receivedReferrers(let referrers): - receivedReferrers(referrers) - case .receivedClicks(let clicks): - receivedClicks(clicks) - case .receivedPublished(let published): - receivedPublished(published) - case .receivedAuthors(let authors): - receivedAuthors(authors) - case .receivedSearchTerms(let searchTerms): - receivedSearchTerms(searchTerms) - case .receivedVideos(let videos): - receivedVideos(videos) - case .receivedCountries(let countries): - receivedCountries(countries) + case .receivedSummary(let summary, let error): + receivedSummary(summary, error) + case .receivedPostsAndPages(let postsAndPages, let error): + receivedPostsAndPages(postsAndPages, error) + case .receivedReferrers(let referrers, let error): + receivedReferrers(referrers, error) + case .receivedClicks(let clicks, let error): + receivedClicks(clicks, error) + case .receivedPublished(let published, let error): + receivedPublished(published, error) + case .receivedAuthors(let authors, let error): + receivedAuthors(authors, error) + case .receivedSearchTerms(let searchTerms, let error): + receivedSearchTerms(searchTerms, error) + case .receivedVideos(let videos, let error): + receivedVideos(videos, error) + case .receivedCountries(let countries, let error): + receivedCountries(countries, error) case .refreshPeriodOverviewData(let date, let period, let forceRefresh): refreshPeriodOverviewData(date: date, period: period, forceRefresh: forceRefresh) case .refreshPostsAndPages(let date, let period): @@ -199,6 +208,7 @@ class StatsPeriodStore: QueryStore { if !isFetchingOverview { DDLogInfo("Stats: All fetching operations finished.") + emitChange() } } @@ -311,7 +321,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching summary.") - self.actionDispatcher.dispatch(PeriodAction.receivedSummary(summary)) + self.actionDispatcher.dispatch(PeriodAction.receivedSummary(summary, error)) } statsRemote.getData(for: period, endingOn: date) { (posts: StatsTopPostsTimeIntervalData?, error: Error?) in @@ -321,7 +331,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching posts.") - self.actionDispatcher.dispatch(PeriodAction.receivedPostsAndPages(posts)) + self.actionDispatcher.dispatch(PeriodAction.receivedPostsAndPages(posts, error)) } statsRemote.getData(for: period, endingOn: date) { (published: StatsPublishedPostsTimeIntervalData?, error: Error?) in @@ -331,7 +341,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching published.") - self.actionDispatcher.dispatch(PeriodAction.receivedPublished(published)) + self.actionDispatcher.dispatch(PeriodAction.receivedPublished(published, error)) } statsRemote.getData(for: period, endingOn: date) { (referrers: StatsTopReferrersTimeIntervalData?, error: Error?) in @@ -341,7 +351,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching referrers.") - self.actionDispatcher.dispatch(PeriodAction.receivedReferrers(referrers)) + self.actionDispatcher.dispatch(PeriodAction.receivedReferrers(referrers, error)) } statsRemote.getData(for: period, endingOn: date) { (clicks: StatsTopClicksTimeIntervalData?, error: Error?) in @@ -351,7 +361,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching clicks.") - self.actionDispatcher.dispatch(PeriodAction.receivedClicks(clicks)) + self.actionDispatcher.dispatch(PeriodAction.receivedClicks(clicks, error)) } statsRemote.getData(for: period, endingOn: date) { (authors: StatsTopAuthorsTimeIntervalData?, error: Error?) in @@ -361,7 +371,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching authors.") - self.actionDispatcher.dispatch(PeriodAction.receivedAuthors(authors)) + self.actionDispatcher.dispatch(PeriodAction.receivedAuthors(authors, error)) } statsRemote.getData(for: period, endingOn: date) { (searchTerms: StatsSearchTermTimeIntervalData?, error: Error?) in @@ -371,7 +381,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching search terms.") - self.actionDispatcher.dispatch(PeriodAction.receivedSearchTerms(searchTerms)) + self.actionDispatcher.dispatch(PeriodAction.receivedSearchTerms(searchTerms, error)) } statsRemote.getData(for: period, endingOn: date) { (videos: StatsTopVideosTimeIntervalData?, error: Error?) in @@ -381,7 +391,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching videos.") - self.actionDispatcher.dispatch(PeriodAction.receivedVideos(videos)) + self.actionDispatcher.dispatch(PeriodAction.receivedVideos(videos, error)) } statsRemote.getData(for: period, endingOn: date) { (countries: StatsTopCountryTimeIntervalData?, error: Error?) in @@ -391,7 +401,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching countries.") - self.actionDispatcher.dispatch(PeriodAction.receivedCountries(countries)) + self.actionDispatcher.dispatch(PeriodAction.receivedCountries(countries, error)) } } @@ -458,7 +468,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching all posts.") - self.actionDispatcher.dispatch(PeriodAction.receivedPostsAndPages(posts)) + self.actionDispatcher.dispatch(PeriodAction.receivedPostsAndPages(posts, error)) self.persistToCoreData() } } @@ -486,7 +496,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching all search terms.") - self.actionDispatcher.dispatch(PeriodAction.receivedSearchTerms(searchTerms)) + self.actionDispatcher.dispatch(PeriodAction.receivedSearchTerms(searchTerms, error)) self.persistToCoreData() } } @@ -514,7 +524,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching videos.") - self.actionDispatcher.dispatch(PeriodAction.receivedVideos(videos)) + self.actionDispatcher.dispatch(PeriodAction.receivedVideos(videos, error)) self.persistToCoreData() } } @@ -542,7 +552,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching all clicks.") - self.actionDispatcher.dispatch(PeriodAction.receivedClicks(clicks)) + self.actionDispatcher.dispatch(PeriodAction.receivedClicks(clicks, error)) self.persistToCoreData() } } @@ -570,7 +580,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching all authors.") - self.actionDispatcher.dispatch(PeriodAction.receivedAuthors(authors)) + self.actionDispatcher.dispatch(PeriodAction.receivedAuthors(authors, error)) self.persistToCoreData() } } @@ -598,7 +608,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching all referrers.") - self.actionDispatcher.dispatch(PeriodAction.receivedReferrers(referrers)) + self.actionDispatcher.dispatch(PeriodAction.receivedReferrers(referrers, error)) self.persistToCoreData() } } @@ -626,7 +636,7 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished fetching all countries.") - self.actionDispatcher.dispatch(PeriodAction.receivedCountries(countries)) + self.actionDispatcher.dispatch(PeriodAction.receivedCountries(countries, error)) self.persistToCoreData() } } @@ -652,7 +662,7 @@ private extension StatsPeriodStore { DDLogInfo("Error fetching all Published: \(String(describing: error?.localizedDescription))") } DDLogInfo("Stats: Finished fetching all published.") - self.actionDispatcher.dispatch(PeriodAction.receivedPublished(published)) + self.actionDispatcher.dispatch(PeriodAction.receivedPublished(published, error)) self.persistToCoreData() } } @@ -692,9 +702,10 @@ private extension StatsPeriodStore { // MARK: - Receive data methods - func receivedSummary(_ summaryData: StatsSummaryTimeIntervalData?) { + func receivedSummary(_ summaryData: StatsSummaryTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingSummary = false + state.fetchingSummaryHasFailed = error != nil if summaryData != nil { state.summary = summaryData @@ -702,9 +713,10 @@ private extension StatsPeriodStore { } } - func receivedPostsAndPages(_ postsAndPages: StatsTopPostsTimeIntervalData?) { + func receivedPostsAndPages(_ postsAndPages: StatsTopPostsTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingPostsAndPages = false + state.fetchingPostsAndPagesHasFailed = error != nil if postsAndPages != nil { state.topPostsAndPages = postsAndPages @@ -712,9 +724,10 @@ private extension StatsPeriodStore { } } - func receivedReferrers(_ referrers: StatsTopReferrersTimeIntervalData?) { + func receivedReferrers(_ referrers: StatsTopReferrersTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingReferrers = false + state.fetchingReferrersHasFailed = error != nil if referrers != nil { state.topReferrers = referrers @@ -722,9 +735,10 @@ private extension StatsPeriodStore { } } - func receivedClicks(_ clicks: StatsTopClicksTimeIntervalData?) { + func receivedClicks(_ clicks: StatsTopClicksTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingClicks = false + state.fetchingClicksHasFailed = error != nil if clicks != nil { state.topClicks = clicks @@ -732,9 +746,10 @@ private extension StatsPeriodStore { } } - func receivedAuthors(_ authors: StatsTopAuthorsTimeIntervalData?) { + func receivedAuthors(_ authors: StatsTopAuthorsTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingAuthors = false + state.fetchingAuthorsHasFailed = error != nil if authors != nil { state.topAuthors = authors @@ -742,9 +757,10 @@ private extension StatsPeriodStore { } } - func receivedPublished(_ published: StatsPublishedPostsTimeIntervalData?) { + func receivedPublished(_ published: StatsPublishedPostsTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingPublished = false + state.fetchingPublishedHasFailed = error != nil if published != nil { state.topPublished = published @@ -752,9 +768,10 @@ private extension StatsPeriodStore { } } - func receivedSearchTerms(_ searchTerms: StatsSearchTermTimeIntervalData?) { + func receivedSearchTerms(_ searchTerms: StatsSearchTermTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingSearchTerms = false + state.fetchingSearchTermsHasFailed = error != nil if searchTerms != nil { state.topSearchTerms = searchTerms @@ -762,9 +779,10 @@ private extension StatsPeriodStore { } } - func receivedVideos(_ videos: StatsTopVideosTimeIntervalData?) { + func receivedVideos(_ videos: StatsTopVideosTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingVideos = false + state.fetchingVideosHasFailed = error != nil if videos != nil { state.topVideos = videos @@ -772,9 +790,10 @@ private extension StatsPeriodStore { } } - func receivedCountries(_ countries: StatsTopCountryTimeIntervalData?) { + func receivedCountries(_ countries: StatsTopCountryTimeIntervalData?, _ error: Error?) { transaction { state in state.fetchingCountries = false + state.fetchingCountriesHasFailed = error != nil if countries != nil { state.topCountries = countries @@ -969,4 +988,31 @@ extension StatsPeriodStore { return state.fetchingPostStats } + var fetchingOverviewHasFailed: Bool { + return state.fetchingSummaryHasFailed && + state.fetchingPostsAndPagesHasFailed && + state.fetchingReferrersHasFailed && + state.fetchingClicksHasFailed && + state.fetchingPublishedHasFailed && + state.fetchingAuthorsHasFailed && + state.fetchingSearchTermsHasFailed && + state.fetchingVideosHasFailed && + state.fetchingCountriesHasFailed + } + + var containsCachedData: Bool { + if state.summary != nil || + state.topPostsAndPages != nil || + state.topReferrers != nil || + state.topClicks != nil || + state.topPublished != nil || + state.topAuthors != nil || + state.topSearchTerms != nil || + state.topCountries != nil || + state.topVideos != nil { + return true + } + + return false + } } From 575a99662c2feaa67f9173a78fa2f96dd1941914 Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Tue, 14 May 2019 15:28:40 +0100 Subject: [PATCH 06/10] =?UTF-8?q?Return=20an=20empty=20table=20view=20if?= =?UTF-8?q?=20there=E2=80=99s=20an=20error=20o=20no=20cached=20data?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Stats/Period Stats/SiteStatsPeriodViewModel.swift | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift index 35d78aef024d..eaae82354b7b 100644 --- a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift +++ b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift @@ -40,6 +40,11 @@ class SiteStatsPeriodViewModel: Observable { var tableRows = [ImmuTableRow]() + if !store.containsCachedData && + (store.fetchingOverviewHasFailed || store.isFetchingOverview) { + return ImmuTable(sections: []) + } + tableRows.append(contentsOf: overviewTableRows()) tableRows.append(contentsOf: postsAndPagesTableRows()) tableRows.append(contentsOf: referrersTableRows()) From dfcff56afabead234f74dc301c454d140aace302 Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Tue, 14 May 2019 15:29:21 +0100 Subject: [PATCH 07/10] Update the tableview every time a change is fired --- .../SiteStatsPeriodTableViewController.swift | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift index e30b29b89b92..72ce8ac8dd8a 100644 --- a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift +++ b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift @@ -109,11 +109,6 @@ private extension SiteStatsPeriodTableViewController { periodDelegate: self) changeReceipt = viewModel?.onChange { [weak self] in - guard let store = self?.store, - !store.isFetchingOverview else { - return - } - self?.refreshTableView() } } @@ -130,20 +125,27 @@ private extension SiteStatsPeriodTableViewController { // MARK: - Table Refreshing func refreshTableView() { - guard let viewModel = viewModel, viewIsVisible() else { return } tableHandler.viewModel = viewModel.tableViewModel() - refreshControl?.endRefreshing() - hideNoResults() + if !store.isFetchingOverview { + refreshControl?.endRefreshing() + } + + if store.containsCachedData { + hideNoResults() + return + } - // Scroll to the top of the table. - // TODO: look at removing this when loading view is added. - tableView.scrollToRow(at: IndexPath(row: 0, section: 0), at: .top, animated: true) + if store.isFetchingOverview { + displayLoadingViewIfNecessary() + } else if store.fetchingOverviewHasFailed { + displayFailureViewIfNecessary() + } } @objc func userInitiatedRefresh() { @@ -191,11 +193,20 @@ extension SiteStatsPeriodTableViewController: NoResultsViewHost { return } - configureAndDisplayNoResults(on: tableView, - title: NoResultConstants.successTitle, - accessoryView: NoResultsViewController.loadingAccessoryView()) { [weak self] noResults in - noResults.delegate = self - noResults.hideImageView(false) + let customizationBlock: NoResultsCustomizationBlock = { [weak self] noResults in + noResults.delegate = self + noResults.hideImageView(false) + } + + if noResultsViewController.view.superview != nil { + updateNoResults(title: NoResultConstants.successTitle, + accessoryView: NoResultsViewController.loadingAccessoryView(), + customizationBlock: customizationBlock) + } else { + configureAndDisplayNoResults(on: tableView, + title: NoResultConstants.successTitle, + accessoryView: NoResultsViewController.loadingAccessoryView(), + customizationBlock: customizationBlock) } } From d2f1776693d1c7372dc7dfbd39680db8d5fc1d9d Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Wed, 15 May 2019 14:55:34 +0100 Subject: [PATCH 08/10] Add listeners to handle the store status --- .../Classes/Stores/StatsPeriodStore.swift | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/WordPress/Classes/Stores/StatsPeriodStore.swift b/WordPress/Classes/Stores/StatsPeriodStore.swift index 6d4baa11a27b..edbd7ed5b6b3 100644 --- a/WordPress/Classes/Stores/StatsPeriodStore.swift +++ b/WordPress/Classes/Stores/StatsPeriodStore.swift @@ -150,6 +150,8 @@ struct PeriodStoreState { } class StatsPeriodStore: QueryStore { + var fetchingOverviewListener: ((_ fetching: Bool, _ success: Bool) -> Void)? + var cachedDataListener: ((_ hasCachedData: Bool) -> Void)? private var statsServiceRemote: StatsServiceRemoteV2? @@ -208,7 +210,7 @@ class StatsPeriodStore: QueryStore { if !isFetchingOverview { DDLogInfo("Stats: All fetching operations finished.") - emitChange() + fetchingOverviewListener?(false, fetchingOverviewHasFailed) } } @@ -237,7 +239,6 @@ class StatsPeriodStore: QueryStore { try? ContextManager.shared.mainContext.save() DDLogInfo("Stats: finished persisting Stats to disk.") } - } // MARK: - Private Methods @@ -314,6 +315,8 @@ private extension StatsPeriodStore { setAllAsFetchingOverview() + fetchingOverviewListener?(true, false) + statsRemote.getData(for: period, endingOn: date) { (summary: StatsSummaryTimeIntervalData?, error: Error?) in if error != nil { DDLogInfo("Error fetching summary: \(String(describing: error?.localizedDescription))") @@ -437,6 +440,8 @@ private extension StatsPeriodStore { DDLogInfo("Stats: Finished setting data to store from Core Data.") } + + cachedDataListener?(containsCachedData) } func refreshPeriodOverviewData(date: Date, period: StatsPeriodUnit, forceRefresh: Bool) { @@ -847,6 +852,7 @@ private extension StatsPeriodStore { } func setAllAsFetchingOverview(fetching: Bool = true) { + state.fetchingSummary = fetching state.fetchingPostsAndPages = fetching state.fetchingReferrers = fetching state.fetchingClicks = fetching @@ -1001,14 +1007,14 @@ extension StatsPeriodStore { } var containsCachedData: Bool { - if state.summary != nil || - state.topPostsAndPages != nil || - state.topReferrers != nil || - state.topClicks != nil || - state.topPublished != nil || - state.topAuthors != nil || - state.topSearchTerms != nil || - state.topCountries != nil || + if state.summary != nil && + state.topPostsAndPages != nil && + state.topReferrers != nil && + state.topClicks != nil && + state.topPublished != nil && + state.topAuthors != nil && + state.topSearchTerms != nil && + state.topCountries != nil && state.topVideos != nil { return true } From a1be167c0d6d8fb299ead86dcaa76f5877ecf2df Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Wed, 15 May 2019 14:56:07 +0100 Subject: [PATCH 09/10] Add Status enum to modify the overview view --- .../SiteStatsPeriodViewModel.swift | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift index eaae82354b7b..5c8d68a8a836 100644 --- a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift +++ b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodViewModel.swift @@ -9,6 +9,7 @@ class SiteStatsPeriodViewModel: Observable { // MARK: - Properties let changeDispatcher = Dispatcher() + var overviewStoreStatusOnChange: ((Status) -> Void)? private weak var periodDelegate: SiteStatsPeriodDelegate? private let store: StatsPeriodStore @@ -32,6 +33,15 @@ class SiteStatsPeriodViewModel: Observable { changeReceipt = store.onChange { [weak self] in self?.emitChange() } + + store.cachedDataListener = { [weak self] hasCacheData in + self?.overviewStoreStatusOnChange?(.fetchingCacheData(hasCacheData)) + } + + store.fetchingOverviewListener = { [weak self] fetching, success in + let status: Status = fetching ? .fetchingData : .fetchingDataCompleted(success) + self?.overviewStoreStatusOnChange?(status) + } } // MARK: - Table Model @@ -68,6 +78,14 @@ class SiteStatsPeriodViewModel: Observable { ActionDispatcher.dispatch(PeriodAction.refreshPeriodOverviewData(date: date, period: period, forceRefresh: true)) self.lastRequestedPeriod = period } + + // MARK: - State + + enum Status { + case fetchingData + case fetchingCacheData(_ hasCachedData: Bool) + case fetchingDataCompleted(_ success: Bool) + } } // MARK: - Private Extension From 6aa6dbbde70e0542e8f95d1439ceb49906da7362 Mon Sep 17 00:00:00 2001 From: Daniele Bogo Date: Wed, 15 May 2019 14:56:23 +0100 Subject: [PATCH 10/10] Implement new closure --- .../SiteStatsPeriodTableViewController.swift | 63 ++++++++++--------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift index 72ce8ac8dd8a..580360bdd94a 100644 --- a/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift +++ b/WordPress/Classes/ViewRelated/Stats/Period Stats/SiteStatsPeriodTableViewController.swift @@ -111,6 +111,31 @@ private extension SiteStatsPeriodTableViewController { changeReceipt = viewModel?.onChange { [weak self] in self?.refreshTableView() } + + viewModel?.overviewStoreStatusOnChange = { [weak self] status in + guard let self = self, + let viewModel = self.viewModel, + self.viewIsVisible() else { + return + } + + self.tableHandler.viewModel = viewModel.tableViewModel() + + switch status { + case .fetchingData: + self.displayLoadingViewIfNecessary() + case .fetchingCacheData(let hasCache): + if hasCache { + self.hideNoResults() + } + case .fetchingDataCompleted(let error): + if error { + self.displayFailureViewIfNecessary() + } else { + self.hideNoResults() + } + } + } } func tableRowTypes() -> [ImmuTableRow.Type] { @@ -126,26 +151,13 @@ private extension SiteStatsPeriodTableViewController { func refreshTableView() { guard let viewModel = viewModel, - viewIsVisible() else { + viewIsVisible(), + !store.isFetchingOverview else { return } tableHandler.viewModel = viewModel.tableViewModel() - - if !store.isFetchingOverview { - refreshControl?.endRefreshing() - } - - if store.containsCachedData { - hideNoResults() - return - } - - if store.isFetchingOverview { - displayLoadingViewIfNecessary() - } else if store.fetchingOverviewHasFailed { - displayFailureViewIfNecessary() - } + refreshControl?.endRefreshing() } @objc func userInitiatedRefresh() { @@ -193,20 +205,15 @@ extension SiteStatsPeriodTableViewController: NoResultsViewHost { return } - let customizationBlock: NoResultsCustomizationBlock = { [weak self] noResults in - noResults.delegate = self - noResults.hideImageView(false) + if noResultsViewController.view.superview != nil { + return } - if noResultsViewController.view.superview != nil { - updateNoResults(title: NoResultConstants.successTitle, - accessoryView: NoResultsViewController.loadingAccessoryView(), - customizationBlock: customizationBlock) - } else { - configureAndDisplayNoResults(on: tableView, - title: NoResultConstants.successTitle, - accessoryView: NoResultsViewController.loadingAccessoryView(), - customizationBlock: customizationBlock) + configureAndDisplayNoResults(on: tableView, + title: NoResultConstants.successTitle, + accessoryView: NoResultsViewController.loadingAccessoryView()) { [weak self] noResults in + noResults.delegate = self + noResults.hideImageView(false) } }