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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Housekeeping! #883

Merged
merged 12 commits into from Sep 18, 2020
Merged

Housekeeping! #883

merged 12 commits into from Sep 18, 2020

Conversation

jleandroperez
Copy link
Contributor

@jleandroperez jleandroperez commented Sep 17, 2020

Details:

  • Encapsulates Options.firstLaunch!!!
  • Drops Legacy PIN Keychain Entry-Check (It's been at least 7 years!!)
  • Drops several unneeded @available checks
  • Drops Migration Handling from 4.8.0 > 4.8.1 (not needed for fresh installs, and upgrading from <4.7 won't break!)
  • Drops dead code!
  • Drops manual Equatable implementations!

@aerych Sir!! Can I bug you with this one?

Code deletion week!!! Yayyy!!!

Test:

  1. Fresh install develop and login
  2. Install this branch on top
  • Verify the app doesn't add a new Welcome Note please!

Test:

  1. Fresh install this branch
  2. Login!
  • Verify the app adds a Welcome Note please 馃槃

Release

These changes do not require release notes.

@jleandroperez jleandroperez added the debt Technical debt. label Sep 17, 2020
@jleandroperez jleandroperez self-assigned this Sep 17, 2020
@peril-automattic
Copy link

peril-automattic bot commented Sep 17, 2020

You can trigger an installable build for these changes by visiting CircleCI here.

@jleandroperez jleandroperez marked this pull request as ready for review September 17, 2020 17:54
Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Hola @jleandroperez 馃憢

Loving all the red in this one :)

I tested as described but.... things didn't quite go as expected. After a fresh install of develop I see a new welcome note. However, after a fresh install of this branch I do not (which seems the opposite of what we're wanting?)

Also, I spotted something curious about the first launch detection/logic and left a note about it.

Back to you sir!

}

return firstLaunch;
return [[Options shared] firstLaunch];
Copy link
Member

Choose a reason for hiding this comment

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

I think something might not be quite right with the changes to the way first launch is detected/checked, Have a look at the logic in didFinishLaunchingWithOptions at line 198 of the diff : https://github.com/Automattic/simplenote-ios/pull/883/files#diff-3d7ffb11fe2a17fede94feba3e9c5aa0R198

When the app is first launched, the value of [[Options shared] firstLaunch] is false so the conditional there will always skip the truth block. Breakpoints and testing after a clean install should confirm (it does for me). Is there some other process that should be setting this flag (that might be bugged) that I'm missing?

Seems like maybe the check should be inverted and maybe renamed for clarity or something in a way that wouldn't conflict with existing installs?

Or maybe just adding back the check to retrieve an object vs a bool and then check for nil would work?

Thoughts?

@jleandroperez
Copy link
Contributor Author

@aerych I'm sorry!!! I had an unpushed commit 馃う

Mind running a quick smoke test, just for sanity?

Thank you sir!!!

Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Ah ha! Pesky missing commit!
Retested after updating and now I see the first launch behavior working as expected. And I do see the welcome note appearing in the new branch. (It's still there in develop too but 馃し I guess that's okay?)

Anyway, :shipit: when ready!

@jleandroperez jleandroperez added this to the 4.25 milestone Sep 18, 2020
@jleandroperez
Copy link
Contributor Author

Thanks a lot for testing this one sir!!!

@jleandroperez jleandroperez merged commit 7e55e32 into develop Sep 18, 2020
@jleandroperez jleandroperez deleted the issue/housekeeping branch September 18, 2020 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants