Add Remote Feature Flag Params to Dashboard Cards endpoint by salimbraksa · Pull Request #674 · wordpress-mobile/WordPressKit-iOS

salimbraksa · 2024-01-03T15:32:03Z

Description

Part of wordpress-mobile/WordPress-iOS#22315

This PR passes the following params to the dashboard cards endpoint:

device_id
platform
build_number
marketing_version
identifier

Testing Details

See wordpress-mobile/WordPress-iOS#22320

Note

Please remove the [Status] DO NOT MERGE label and merge after:

the backend change is deployed D133371-code
the WPiOS PR is approved so that the two PRs are merged together and no breakages is introduced in trunk

Please check here if your pull request includes additional test coverage.
I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

…xistance

…e RemoteFeatureFlagsEndpointParams Add a new getRemoteFeatureFlags variant that accepts an object of type RemoteFeatureFlagsEndpointParams

hassaanelgarem

Looks good to me! 🚀
Kudos on adding tests!
Only left a few code improvement suggestions.

hassaanelgarem · 2024-01-05T03:02:48Z

+            "cards": cards.joined(separator: ",") as NSString
+        ]
+        let featureFlagParams = try {
+            let params = FeatureFlagRemote.RemoteFeatureFlagsEndpointParams(deviceId: deviceId)


I wonder if this struct should still live inside FeatureFlagRemote now that it's shared with an unrelated class.

That was the plan initially, but couldn't figure out a name for that type. 😅

Co-authored-by: hassaanelgarem <hassaanelgarem@gmail.com>

…ithub.com:wordpress-mobile/WordPressKit-iOS into task/22315-backend-driven-dynamic-cards-filtering

hassaanelgarem · 2024-01-05T15:55:01Z

    }

-    public struct RemoteFeatureFlagsEndpointParams {
+    public struct FetchAllEndpointParams {


@salimbraksa FetchAllEndpointParams feels a bit confusing. How about SessionDetails?

Also, can we move it outside of the FeatureFlagRemote scope? Referencing it by FeatureFlagRemote.FetchAllEndpointParams hurts the readability as it implies it is tighly coupled with feature flags while it's not. This is what I meant here #674 (comment)

My concern with global scope is the following:

Let's say another developer added a valid "session" attribute in SessionDetails.

The new attribute is encoded and returned in the dictionary representation.

Now that attribute will be sent in all other endpoints, and I'm not sure how endpoints handle extra attributes that don't need.

This is why I kept that type tightly related to Feature Flags, and if you think about it, the new params added in Dashboard Cards are for the Feature Flags mechanism.

Now that attribute will be sent in all other endpoints, and I'm not sure how endpoints handle extra attribute that don't need.

We can assume that developers will seek to understand what this type is used for before changing it.
However, extra params won't cause an issue either way.

We can assume that developers will seek to understand what this type is used for before changing it.

I agree, but my concern is that if we expand the SessionDetails with new 'session' attributes in the future, we need to ensure they are not inadvertently included in the Feature Flags and Dashboard Cards endpoint query params.

If we rename that type to SessionDetails, I think the Feature Flag Remote and Dashboard Cards service should select only the params they need.

Also, while I'm sure developers double check API usage before making changes, I think it's also important to write resilient code that mitigates the risks associated with human error.

I'll rename the type to SessionsDetails and update the Unit Tests to ensure only the the params required by the endpoint are sent.

However, extra params won't cause an issue either way.

Given this, I think we would be over-engineering to avoid something that's not critical.

Resolved in 28cc006

However, extra params won't cause an issue either way.

Passing extra params to endpoints is a code smell in my opinion. Moreover, some backend APIs may reject the request if it includes additional parameters as a security measure.

I agree that we shouldn't spend time on imaginary problems. For now, it's enough to have Unit Tests to check for this. I looked at the tests we already have, and they'll fail if extra params are sent to Feature Flags and Dashboard Cards endpoints.

mokagio · 2024-01-08T00:17:27Z

Thank you for working on this @salimbraksa and for adding tests. I also appreciate the helper functions, which enable writing othre tests in the future 🎉

I landed here while preparing the library for Jetpack and WordPress iOS 24.0. I noticed this PR introduce a breaking change, which is something we should be careful with because of the downstream implication in https://github.com/wordpress-mobile/WordPressAuthenticator-iOS which depends on WordPressKit.

Given the app PR that uses this logic, wordpress-mobile/WordPress-iOS#22320, has not been merged at this time and won't be part of 24.0, I will not ship a new release with this breaking change just yet.

Ideally, we'd want to provide a non-breaking API. If that's not possible, given we'll need to go through the effort of shipping the breaking change, we might as well look for other breaking low hanging fruits to slot in.

Cheers (and Happy New Year!)

salimbraksa · 2024-01-08T00:33:27Z

Hey @mokagio, the other PR was supposed to be merged on Friday, but I just realized it didn't because of a flaky unit test unrelated to my PR.

The PR was already been reviewed and approved, so I’ll work on fixing the failing test and aim to merge it into release/24.0.

The parameter was [initially designed to be non-optional][PR-1]. Later it's later [changed to be optional][PR-2], under [this proposal][comment-1]. This commit simply reverts the parameter to be non-optional, to ship it along with other breaking changes. [PR-1]: #674 [PR-2]: #676 [comment-1]: wordpress-mobile/WordPress-iOS#22320 (comment)

salimbraksa added 5 commits January 3, 2024 16:30

Add unit test checking Get Remote Feature Flag endpont query params e…

5dd5afb

…xistance

Add a new getRemoteFeatureFlags variant that accepts an object of typ…

d212c99

…e RemoteFeatureFlagsEndpointParams Add a new getRemoteFeatureFlags variant that accepts an object of type RemoteFeatureFlagsEndpointParams

Add unit test checking Dashboard Cards endpoint and query params

4da0768

Refactor DashboardServiceRemote fetch method

6e6254a

Update DashboardServiceRemote fetch method to accept a deviceId param

1f7ab13

salimbraksa force-pushed the task/22315-backend-driven-dynamic-cards-filtering branch from 7239eef to 1f7ab13 Compare January 3, 2024 18:12

salimbraksa self-assigned this Jan 3, 2024

salimbraksa added the enhancement New feature or request label Jan 3, 2024

salimbraksa mentioned this pull request Jan 3, 2024

Backend-driven Dynamic Cards filtering wordpress-mobile/WordPress-iOS#22320

Merged

4 tasks

salimbraksa added the [Status] DO NOT MERGE label Jan 3, 2024

salimbraksa marked this pull request as ready for review January 3, 2024 23:20

salimbraksa requested review from alpavanoglu and hassaanelgarem January 3, 2024 23:20

Update CHANGELOG

fa7d74f

salimbraksa force-pushed the task/22315-backend-driven-dynamic-cards-filtering branch 7 times, most recently from 34cdcad to 91b6e05 Compare January 5, 2024 02:24

Update testRequestCardsParam

accfc22

salimbraksa force-pushed the task/22315-backend-driven-dynamic-cards-filtering branch from 91b6e05 to accfc22 Compare January 5, 2024 02:30

hassaanelgarem approved these changes Jan 5, 2024

View reviewed changes

salimbraksa and others added 5 commits January 5, 2024 16:23

Update WordPressKit/FeatureFlagRemote.swift

8fcb113

Co-authored-by: hassaanelgarem <hassaanelgarem@gmail.com>

Move feature flag params encoding to helper method

90bbe5e

Merge branch 'task/22315-backend-driven-dynamic-cards-filtering' of g…

3b3f7eb

…ithub.com:wordpress-mobile/WordPressKit-iOS into task/22315-backend-driven-dynamic-cards-filtering

Rename featue flag endpoint params type

d13dbe8

Use dictionaryRepresentation method in more locations

ca469ac

hassaanelgarem reviewed Jan 5, 2024

View reviewed changes

salimbraksa added 3 commits January 5, 2024 17:46

Move Feature Flag Params to global scope

28cc006

Fix lint warning

8891881

Fix minor typo

432a336

salimbraksa removed the [Status] DO NOT MERGE label Jan 5, 2024

salimbraksa merged commit 87eba25 into trunk Jan 5, 2024

salimbraksa deleted the task/22315-backend-driven-dynamic-cards-filtering branch January 5, 2024 17:18

mokagio mentioned this pull request Jan 8, 2024

Bump version to 9.0.2 #675

Merged

crazytonyli mentioned this pull request Jan 11, 2024

Make deviceId a required parameter #678

Merged

2 tasks

Uh oh!

Conversation

salimbraksa commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Details

Note

Uh oh!

hassaanelgarem left a comment

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hassaanelgarem Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Jan 8, 2024

Uh oh!

salimbraksa commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

salimbraksa commented Jan 3, 2024 •

edited

Loading

salimbraksa Jan 5, 2024 •

edited

Loading

salimbraksa Jan 5, 2024 •

edited

Loading