Conversation
JSON parsing is still quite unstable and can crash on self hosted sites with older Jetpack versions.
Moved storage to core data and use a NSFetchedResultsController for change notification
I wanted to move networking code out of the view layer, but let's start with something that works and build from there.
|
I've had a look at the PR, unfortunately I haven't done swift for a very long time, so I wasn't sure about a lot of stuff. I just wanted to expose myself a little bit at the swift code and also take a look at what you have done in iOS side since I am working on Android version myself. I don't really have much to contribute review-wise. Having said that, I've tested the branch a bit and found an issue with a Jetpack blog I have. In wp-admin my user is set as "Administrator" whereas the app shows me in "Unsupported" role. You can find screenshots for it below. Let me know if I can help with the testing phase any further and I'll be looking for Sendhil's comments for the code review!
|
|
After chatting with @koke on Slack, we found out that the issue with the Role not showing up was that my Jetpack was not up to date, so no problems with the PR. |
There was a problem hiding this comment.
This is a little nit picky so feel free to ignore it, but the _ in naming doesn't feel very sSwiftish. Perhaps rename to firstName, lastName, etc.?
|
You probably know this but right now no error message gets displayed when a user tries to view a people list of a site they don't have permission to. That being said I do see the error being logged to the console. |
There was a problem hiding this comment.
I'm not sure about this one. If we're at the point where we're ready to call the success block, I really want to call mergeTeam so we don't waste the downloaded data.
When I run Instruments I don't see any leaks or retain cycles
There was a problem hiding this comment.
I took a different approach and made PeopleService a struct. I'm not 100% sure, but from what I understand, it is now a value type, so there are no reference cycles: the callback just gets a copy of the service.
I also think this matches what I wanted more closely. I think I changed it to a class because I planned to keep track of requests in progress, but I don't think that's happening in the short term
|
@koke sorry on the delay - left a few comments for you. |
I didn't 😄 Added it to the PR known issues list for reference when we resume development. Also merged develop and resolved conflicts. Ready for the next round |
|
Looks good to me @koke |
First PR for People management
Note that there's also some work done on viewing and editing users in #4309, but since we're putting the feature on hold for a bit, I'm trying to get what's working merged into develop.
For the feature flag, I went with a
#defineonConstants.h. Usually it'd be my least preferred method, but it felt like the simplest way to enable this on Debug builds only without adding a lot of logic to the Blog Details table view sections/rows.I decided to keep this enabled in Debug so it's easy to test if we need to change code. I remember some code not accessible from the UI (themes, media browser) that got left out of refactors because it wasn't visible.
Known issues
Needs Review: @sendhil