Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Make the deviceId argument optional in the DashboardServiceRemote.deviceId method - #676

Merged
mokagio merged 1 commit into
trunkfrom
task/22315-make-device-id-optional
Jan 9, 2024
Merged

Make the deviceId argument optional in the DashboardServiceRemote.deviceId method#676
mokagio merged 1 commit into
trunkfrom
task/22315-make-device-id-optional

Conversation

@salimbraksa

@salimbraksa salimbraksa commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

Description

Ref wordpress-mobile/WordPress-iOS#22320 (comment)

This PR updates the DashboardServiceRemote.deviceId method to make the deviceId argument optional to prevent breaking users of this library.

Testing Details

N/A


  • 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.

@salimbraksa salimbraksa self-assigned this Jan 9, 2024
@salimbraksa
salimbraksa marked this pull request as ready for review January 9, 2024 02:14
@salimbraksa

salimbraksa commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

Hey @mokagio, in response to your comment, I've updated the code to make the deviceId parameter optional as you suggested. Ideally, the deviceId should be included in the query params, but it was made optional for backward compatibility ( users running older Jetpack version ).

@salimbraksa salimbraksa changed the title Turn deviceId argument of DashboardServiceRemote.deviceId optional Make the deviceId argument optional in the DashboardServiceRemote.deviceId method. Jan 9, 2024
@salimbraksa salimbraksa changed the title Make the deviceId argument optional in the DashboardServiceRemote.deviceId method. Make the deviceId argument optional in the DashboardServiceRemote.deviceId method Jan 9, 2024

@hassaanelgarem hassaanelgarem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@mokagio mokagio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @salimbraksa !

Comment on lines +39 to +42
let featureFlagParams: [String: AnyObject]? = try {
guard let deviceId else {
return nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, interesting. I didn't realize it would be possible to handle a missing deviceId in this way. Looks like a nice way to keep the app "working".

@mokagio
mokagio merged commit bee91b4 into trunk Jan 9, 2024
@mokagio
mokagio deleted the task/22315-make-device-id-optional branch January 9, 2024 02:41
crazytonyli added a commit that referenced this pull request Jan 11, 2024
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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants