Skip to content
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

MSD - Stats Card - Handle Success Response #2244

Merged
merged 10 commits into from
Feb 18, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Jan 17, 2022

Parent wordpress-mobile/WordPress-Android#15749

This is a draft PR that includes the following changes for adding the Stats Card to the cards result:

  • JSON deserialization for the success response
  • Mapping success response to model

There's nothing to test in this PR as the dashboard/cards endpoint doesn't yet include the Stats Card yet. So that existing tests work as expected, I've updated the following test classes:

  • CardsRestClientTest is updated to add StatsResponseand cards.json is updated to include the Stats Card.
  • CardsStoreTest is updated to add STATS_ENTITY to CARDS_ENTITY.

Note: We might want to keep this PR in draft mode until the dashboard/cards endpoint is updated for the Stats Card as the final structure might vary. With the current prod version of the endpoint, the stats card response being null is not added to the cards list.

@zwarm
Copy link
Contributor

zwarm commented Jan 17, 2022

We might want to keep this PR in draft mode

@ashiagr , yes, let's do that!
(1) I added comments to the endpoint because it's needed for iOS; not sure if we want to do anything with it.
(2) Let's sync on what the success looks like ... I was basing it off of the following, but that can be changed.


Success: return array(
        'ok'           => true,
	'error'      => '',
	'stats'      => stats data,
      );
Failure: return array(
        'ok'     => false,
	 'error' => 'error message',
      );

NOTE:: We decided against the above approach, this is here for history purposes

- Added comments to todays_stats.
- Moved nested todays_stats to keep it at the same level as posts node in cards.json and corresponding response and model classes.

Note that the existing endpoint will not return the stats card yet. It will be switched to a newer version in future commits.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 26, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@AjeshRPai AjeshRPai closed this Feb 10, 2022
@AjeshRPai AjeshRPai reopened this Feb 10, 2022
@zwarm zwarm assigned zwarm and ashiagr and unassigned ashiagr Feb 16, 2022
@zwarm
Copy link
Contributor

zwarm commented Feb 16, 2022

@ashiagr - The cards-data endpoint has been deployed - you can swap out the mock data responses.

@ashiagr ashiagr marked this pull request as ready for review February 17, 2022 09:27
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashiagr Very well done 👏

❤️ for the patience in changing all those tests.

I like the improvement that added "label" to types so the arguments build out works smoothly.

I tested using the WPAndroid#[15893 ](MSD - Handle Single Dashboard Card Error in UI) and was able to view the stats card as expected. I also visited a few web sites and PTR's, and the counts updated.

Please feel free to merge when you feel comfortable.

@ashiagr ashiagr merged commit 3159174 into trunk Feb 18, 2022
@ashiagr ashiagr deleted the issue/15749-msd-stats-card-handle-success-response branch February 18, 2022 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants