Spaces: Add support for spaces Summary API #1072
Conversation
MatrixSDK/Space/MXSpace.swift
Outdated
| /// - completion: A closure called when the operation completes. Provides the event id of the event generated on the home server on success. | ||
| /// - Returns: a `MXHTTPOperation` instance. | ||
| @discardableResult | ||
| public func addChildWithDefaultViaServer(spaceId: String, |
There was a problem hiding this comment.
Why not remove this method to limit methods number and duplicated code? This will improve readiness and future maintenance.
Having an optional viaServers in the previous method will fulfil the behavior of this method.
| import Foundation | ||
|
|
||
| /// MXSpaceChildInfo represents space child summary informations. | ||
| @objcMembers |
There was a problem hiding this comment.
Just a question: do we need usage from objc?
There was a problem hiding this comment.
Yes this class will soon be exposed from MXRoomSummary
|
|
||
| if (spaceChildSummaryResponse) | ||
| { | ||
| MXJSONModelSetString(spaceChildSummaryResponse.roomId, JSONDictionary[@"room_id"]); |
There was a problem hiding this comment.
We should return nil if there is no roomId else we do not respect non_null in the header.
MatrixSDK/Space/MXSpaceService.swift
Outdated
| return self.session.matrixRestClient.getSpaceChildrenForSpace(withId: spaceId, parameters: parameters) { (response) in | ||
| switch response { | ||
| case .success(let spaceChildrenResponse): | ||
| guard let rooms = spaceChildrenResponse.rooms else { |
There was a problem hiding this comment.
Should we have a processing queue?
I heard that we can expect crazy spaces hierarchies.
| roomSummary.roomTypeString = roomTypeString | ||
| roomSummary.roomType = self.roomTypeMapper.roomType(from: roomTypeString) | ||
|
|
||
| let membersCount = MXRoomMembersCount() |
There was a problem hiding this comment.
What about MXRoomMembersCount.members and MXRoomMembersCount.invited?
We should have members >= joined.
|
|
||
| case .failure(let error): | ||
| XCTFail("Add child space failed with error \(error)") | ||
| } |
| if spaces.count == spaceNames.count { | ||
| completion(.success(spaces)) | ||
| } else { | ||
| XCTFail("Fail to create spaces") |
There was a problem hiding this comment.
We should return an error to avoid blocking the test suite. See other comments in this file.
|
|
||
| expectation.fulfill() | ||
| case .failure(let error): | ||
| XCTFail("Get space children failed with error \(error)") |
| @@ -169,4 +202,179 @@ class MXSpaceServiceTest: XCTestCase { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| /// - Create Bob | |||
There was a problem hiding this comment.
Can you report these comments in the code?
This is absolutely boring but it is even more boring to understand what a test does when you need to debug it afterwards.
| } | ||
| } | ||
|
|
||
| /// - Create Bob |
There was a problem hiding this comment.
report these comments in the code
…Expectation.fulfill() calls.
… add both space or room as child.
…te an array of spaces.
| if (spaceChildSummaryResponse) | ||
| MXJSONModelSetString(roomId, JSONDictionary[@"room_id"]); | ||
|
|
||
| if (spaceChildSummaryResponse && roomId) |
There was a problem hiding this comment.
We should return nil if there is no roomId else we can have a non valid MXSpaceChildSummaryResponse instance at runtime.
MatrixSDK/Space/MXSpace.swift
Outdated
| public func addChild(spaceId: String, | ||
| viaServers: [String], | ||
| public func addChild(roomId: String, | ||
| viaServers: [String]?, |
There was a problem hiding this comment.
I wonder if can have nil as default value. We are in full swift here, no?
| let childSpace = spaces[1] | ||
|
|
||
| // Add space A as child of space B | ||
| rootSpace.addChild(roomId: childSpace.spaceId, viaServers: nil, order: nil, autoJoin: false, suggested: false) { (response) in |
There was a problem hiding this comment.
Now that we have default params values, we can do some cleaning ;)
Support MSC2946.
element-hq/element-ios/issues/4068