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

Use correct consumer key on iPad #267

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Use correct consumer key on iPad #267

merged 1 commit into from
Apr 12, 2023

Conversation

dskuza
Copy link
Contributor

@dskuza dskuza commented Sep 22, 2022

Summary

Use the "legacy" consumer key for "pad" idioms. This should not be merged until all the appropriate bitrise config files have been updated as well.

References

Implementation Details

  • Add new row to Info.plist for both PocketKit and SaveToPocketKit
  • Update Keys.swift in both PocketKit and SaveToPocketKit to obtain both consumer keys, and set the appropriate key based on the device's user interface idiom.

Note: the key contains the word "pad" vs "iPad" because the idiom is "pad", and I wanted the two to match.

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

@dskuza dskuza requested a review from a team as a code owner September 22, 2022 18:58
Copy link
Member

@bassrock bassrock left a comment

Choose a reason for hiding this comment

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

Works for me, what is the experience like going from an old build to this? Does it auto log users out or does it error?

@@ -54,6 +54,8 @@
<string>$(POCKET_API_BASE_URL)</string>
<key>PocketAPIConsumerKey</key>
<string>$(POCKET_API_CONSUMER_KEY)</string>
<key>PocketAPIConsumerKeyPad</key>
<string>$(POCKET_API_CONSUMER_KEY_PAD)</string>
Copy link
Member

Choose a reason for hiding this comment

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

Note add to bitrise env variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bassrock I believe we discussed this live and determined we would perform no other action at this time.

Copy link
Member

Choose a reason for hiding this comment

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

@dskuza right, but would still like to understand what the experience is going to do so it can be communicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bassrock Fair, I missed the second part of your comment somehow 😅 . I can test that path, but I believe things will just seem to … not … be … working? Off to verification land…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bassrock Looks like things like pull-to-refresh on My List fail and spin, and empties the list. Looks like at least one resolution would be communicate to iPad users that if they're seeing issues to log out and log back in (which I still think is okay since we're in beta - this isn't something that would affect a production release).

Copy link
Member

Choose a reason for hiding this comment

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

@dskuza cool, yep given this would make the next beta if merged today, and @nzeltzer will be emailing folks, let's post that in the slack channel for the team to know.

Copy link
Member

@bassrock bassrock left a comment

Choose a reason for hiding this comment

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

Approved, pending the slack heads up

@dskuza
Copy link
Contributor Author

dskuza commented Sep 22, 2022

Although approved, do not yet merge this pull request.

@dskuza dskuza added the 🛑 DO NOT MERGE Do not merge label Sep 22, 2022
@dskuza dskuza marked this pull request as draft September 22, 2022 19:21
Copy link
Contributor

@jacobsimeon jacobsimeon left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@dskuza dskuza force-pushed the ipad-consumer-key branch 2 times, most recently from 4ac0926 to a57d244 Compare April 12, 2023 21:03
@dskuza dskuza removed the 🛑 DO NOT MERGE Do not merge label Apr 12, 2023
@dskuza dskuza marked this pull request as ready for review April 12, 2023 21:16
@dskuza dskuza merged commit aef9b75 into develop Apr 12, 2023
@dskuza dskuza deleted the ipad-consumer-key branch April 12, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants