-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix ghost not disappearing from Insights cards in certain conditions #18104
Conversation
…the CoreData fetch returned valid data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Gio2018 the direction + outcome of all the new state status(es) being set need to be reversed.
Example for a single status - it should be like:
state.allTimeStats != nil ? .success : .error
OR
state.allTimeStats == nil ? .error : .success
Instead of:
state.allTimeStats != nil ? .error : .success
I made this mistake the first time and this was my first commit before I corrected it so I'm sure that's what has transpired.
Link to my correction here
FYI, #18119, which addresses the UI tests issue, has been merged in the release branch. A merge or rebase here should address the UI tests failures because of the "No data yet" and move along testing the new logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…rdPress-iOS into issue-17904-insights-cards-not-showing
@Gio2018 this has been bundled as part of 19.4 beta 2 (19.4.0.2). Thanks for your work 🙌 |
Fixes #17904
To test:
Pre-requisite: use a wp.com account with one or more site that have stats, preferably with many accesses
Testing:
Regression Notes
Potential unintended areas of impact
Stats -> Insights
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual test and run UI tests
What automated tests I added (or what prevented me from doing so)4.
Some changes to the UI tests
PR submission checklist:
RELEASE-NOTES.txt
if necessary.