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

[Unticketed] user needs_password not returned in v1/discover/ creator #1758

Merged
merged 2 commits into from Nov 28, 2022

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Nov 28, 2022

πŸ“² What

Noticed a small regression in web-666 where User.needsPassword was documented as being non-optional in the v1 swagger docs but in other v1 calls that return a user object the field is missing entirely.

Example:
/v1/discover?client_id=1HP13YYFC4N278H30RJYKPAQR3H8KK4SF6IWI0JPXHPRP46VWN&currency=USD&sort=magic

		"creator": {
			"id": 357970774,
			"name": "Rebecca Lemme",
			"slug": "actsofmatter",
			"is_registered": null,
			"is_email_verified": null,
			"chosen_currency": null,
			"is_superbacker": null,
			"avatar": {
				"thumb": "https://ksr-ugc.imgix.net/",
				"small": "https://ksr-ugc.imgix.net/",
				"medium": "https://ksr-ugc.imgix.net/"
			},
			"urls": {
				"web": {
					"user": "https://www.kickstarter.com/profile/actsofmatter"
				},
				"api": {
					"user": "https://api.kickstarter.com/v1/users/357970774?signature=1669755294.e10ce7f978c62fa789273d34c52089a8c49f0232"
				}
			}
		},

πŸ€” Why

v1 API is seemingly not well documented which makes sense given it is being phased out.

πŸ›  How

Made the User.needsPassword property optional.

πŸ‘€ See

Trello, screenshots, external resources?

Before πŸ› After πŸ¦‹
Simulator Screen Recording - iPhone 8 - 2022-11-28 at 16 06 27 Simulator Screen Recording - iPhone 8 - 2022-11-28 at 16 16 11

βœ… Acceptance criteria

  • Ensure projects load when app is opened (ie. v1/discover/)

⏰ TODO

  • Ensure tests pass

@msadoon msadoon added this to the release-5.7.0 milestone Nov 28, 2022
@msadoon msadoon self-assigned this Nov 28, 2022
@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #1758 (975e01c) into main (611cc91) will increase coverage by 0.00%.
The diff coverage is 40.00%.

@@           Coverage Diff           @@
##             main    #1758   +/-   ##
=======================================
  Coverage   85.42%   85.42%           
=======================================
  Files        1273     1273           
  Lines      116200   116201    +1     
  Branches    30634    30635    +1     
=======================================
+ Hits        99268    99269    +1     
+ Misses      15866    15865    -1     
- Partials     1066     1067    +1     
Impacted Files Coverage Ξ”
...LoginTout/Controller/LoginToutViewController.swift 56.16% <0.00%> (-0.15%) ⬇️
KsApi/models/User.swift 81.96% <50.00%> (-0.82%) ⬇️
KsApi/models/UserTests.swift 97.00% <100.00%> (ΓΈ)
Library/Navigation.swift 85.85% <0.00%> (+0.64%) ⬆️

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

My bad. Tried to approve from the iOS app but was logged in on my personal account.

@msadoon msadoon merged commit 36fdf25 into main Nov 28, 2022
@msadoon msadoon deleted the fix/needs-password-non-optional branch November 28, 2022 21:56
scottkicks added a commit that referenced this pull request Dec 6, 2022
* [PAY-2055] Project Currency Country Code Display Error (#1754)

* added a project currency, oddly build is crashing when opening a project, but unrelated to any new code.

* removed extaneous currency field from top level project and used existing stats field.

* fixed incorrect parameter build issue

* fixed extra parameter in lenses

* removed unused line

* corrected display issues for rewards and reward addons.

* reverting spacing in ProjectLenses, unneeded change, cluttering pr

* more ProjectLenses updated

* bonus support currency fix on pledge page.

* updated a shared function of min max pledge amount to use the project's currency country instead of project country.

* updated Apple Pay request currency code to be from project currency first, then project country currency. Also updated the supported networks of apple pay to be based on project currency country instead of project country

* updated analytics to come from the project currency country not project country

* updated summary view, apple pay alert, and expandable rewards header (spacing) to use project currency country first then project country

* total values for pledge summary view using project currency country before project country

* handled logged currency display scenarios (manage pledge flow)

* formatting

* fixed currency issue when user logged in and about to manage pledge, fetch backing doesn't update the chosen currency in the project.

* added view model tests

* more view model tests

* project summary view model tests

* pledge view model and project page view model tests

* project page snapshot tests added, existing project lense hadn't accounted for extended project properties, now it does.

* pr comments.

* [WEB-666] - Require FB Only Users To Set A Password  (#1755)

* adds  bool to user model

* separate apple and facebook signals to gain more control over facebook flow in LoginToutViewController

* Push SetYourPasswordViewController if fb user needs a password

* Run make strings script to pull in latest string updates from server

* fetch user's email and set context label

* create new password on user account and handle success and error cases

* make strings

* change nav bar back button text

* update unit tests

* wrap in fb login depreciated feature flag

* update view model tests

* adds activity indicator

* update strings

* add tests

* format

* update failing snapshot tests

* needsPassword is not actually an optional

* use kickstarter conventions

* updates uses of now non nil needsPassword property

* move loading indicator from top level view to stackview and replace save button when loading indicator is showing

* update snapshot tests

* update comments

* update snapshot tests

* update saveButton isHidden/isEnabled

* Remove unused User+GraphUser.swift

* Update User+UserFragment.swift

* Removes User+UserGraph releated tests

* Update LoginToutViewModelTests

* Update SetYourPasswordViewModelTests

* formatting

* Refactor needsPassword static func

* put back User+GraphUser because more test files than expected use it

* Revert "Removes User+UserGraph releated tests"

This reverts commit 2026508.

* corrected the api and library tests, was able to remove unused User+GraphUser and tests. Snapshot tests still failing.

* updated snapshot tests, all kickstarter-ios tests recorded and passing on intel. Should pass CI checks.

* revoked a change just being used for testing.

* rerecorded tests due to translation changes

* more re-records on Intel because of translations

* updated set your view model tests

Co-authored-by: Mubarak Sadoon <msadoon@gmail.com>

* [Unticketed] user needs_password not returned in v1/discover/ creator (#1758)

* needs password needs to be optional.

* add a default property for encoding

* [PAY-2053] Add New Payment Button Padding FIx (#1759)

* added constraints to add new button and loading indicator view to constraint them to the edges of their stack view and center them. One is only shown at a time, so the spacing error should not occur anymore.

* slight adjustment in the add button and loading indicator positions.

* updates to pledge view controller screenshot tests.

* guard against index out of range issue and default selected tab to index 0 if this scenario arises (#1761)

Co-authored-by: Mubarak Sadoon <msadoon@gmail.com>
@msadoon msadoon modified the milestones: release-5.7.0, release-5.6.0 Feb 13, 2023
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.

None yet

3 participants