WordPress-iOS icon indicating copy to clipboard operation
WordPress-iOS copied to clipboard

Published 20 hours ago • verified publisher icon wordpress-mobile

Remove AccountService from all the things

Open jkmassel opened this issue 3 years ago • 16 comments

Disclaimer: this PR description is written by @crazytonyli, not Jeremy. 😸

Changes

The main change is in AccountService, removing a few account lookup APIs from it. The rest changes are making the migration, using the APIs in WPAccount+Lookup instead. Even though there are lots of files are changed, but the changes are quite similar, I would recommend utilize the "Viewed" checkbox at top right of each file in the PR diff page—it sure helped me reviewing this PR.

Test Instructions

Making sure features only available for logged in user are working as expected, and vise-versa. i.e. user is able to edit draft in their sites, not in others' sites.

PR submission checklist:

  • [x] I have considered adding unit tests where possible. N/A. No new API is added in this PR
  • [x] I have considered adding accessibility improvements for my changes. N/A
  • [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. Not Needed

jkmassel avatar Mar 26 '21 06:03 jkmassel

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

peril-wordpress-mobile[bot] avatar Mar 26 '21 06:03 peril-wordpress-mobile[bot]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile[bot] avatar Mar 26 '21 21:03 peril-wordpress-mobile[bot]

Pushing this PR to the next milestone, 17.9, because I'm running the code-freeze today and this is still a draft.

mokagio avatar Jul 12 '21 01:07 mokagio

Pushing this PR to the next milestone, 18.0, because I'm running the code-freeze today and this is still a draft.

@jkmassel is this a change that we can implement in pieces? It might make it easier to get the ball rolling?

mokagio avatar Jul 26 '21 01:07 mokagio

Pushing this PR to the next milestone, 18.1, because I'm running the code-freeze today and this is still a draft.

@jkmassel is this a change that we can implement in pieces? It might make it easier to get the ball rolling? How can I help?

mokagio avatar Aug 09 '21 00:08 mokagio

Pushing this PR to the next milestone, 18.2, because I'm running the code-freeze today and this is still a draft.

mokagio avatar Aug 23 '21 00:08 mokagio

Pushing this PR to the next milestone, 18.3, because I'm running the code-freeze today and this is still a draft.

mokagio avatar Sep 06 '21 00:09 mokagio

Pushing this PR to the next milestone, 18.4, because I'm running the code-freeze today and this is still a draft.

mokagio avatar Sep 20 '21 02:09 mokagio

Pushing this PR to the next milestone, 18.5, because I'm running the code-freeze today and this is still a draft.

mokagio avatar Oct 04 '21 01:10 mokagio

Pushing this PR to the next milestone, 18.6, because I'm running the code-freeze today and this is still a draft.

mokagio avatar Oct 17 '21 23:10 mokagio

After bumping this to the next milestone for more than 10 release cycles, I think it's time to park this in Someday.

mokagio avatar Nov 15 '21 02:11 mokagio

You can test the changes in Jetpack from this Pull Request by:

  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr16168-657c3dd on your iPhone
If you need access to App Center, please ask a maintainer to add you.

wpmobilebot avatar Jul 28 '22 05:07 wpmobilebot

You can test the changes in WordPress from this Pull Request by:

  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr16168-657c3dd on your iPhone
If you need access to App Center, please ask a maintainer to add you.

wpmobilebot avatar Jul 28 '22 05:07 wpmobilebot

Warnings
:warning: PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by :no_entry_sign: dangerJS

peril-wordpress-mobile[bot] avatar Aug 01 '22 08:08 peril-wordpress-mobile[bot]

Thanks for the review, @mokagio .

@jkmassel can probably talk about the grand plan. But in my understanding, yes, DI would be one of the end goals.

I wasn't thinking about self-hosted sites, but good point, it's definitely worth checking! I was mainly thinking about WP.com features, like posts you see in Reader shouldn't show an "Edit" button if the logged in user doesn't have permission to do so, etc.

crazytonyli avatar Aug 01 '22 09:08 crazytonyli

@mokagio That's okay. I also forgot to push up the commits that address your comments. 🥲

crazytonyli avatar Aug 10 '22 05:08 crazytonyli

@crazytonyli I'm ok to merge this, though we'll need to sort out a merge conflict again (sorry!)

Because my name is on the PR I don't think I can approve it, but feel free to merge when it's clean.

jkmassel avatar Aug 12 '22 21:08 jkmassel