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.

Sign up for GitHub

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

Jump to bottom

feat: limited v7 API integration with individual to team migration endpoint - WPB-11961 #2151

Open
wants to merge 3 commits into
base: feat/individual-to-team-migration-ui
Choose a base branch
from
Open

feat: limited v7 API integration with individual to team migration endpoint - WPB-11961 #2151

wants to merge 3 commits into from
+917 −27

Conversation

El-Fitz
Copy link
Contributor

@El-Fitz El-Fitz commented Nov 12, 2024 •
edited by github-actions bot
Loading

TaskWPB-11961 [iOS] add API endpoint for upgrading to a team

Issue

Please describe the issue.

A barebones v7 API integration, limited to adding the individual to team migration endpoint, using APIService

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

  • Make sure you use the API for UI elements that support large fonts.
  • All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
  • New UI elements have Accessibility strings for VoiceOver.

@El-Fitz El-Fitz self-assigned this Nov 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited
Loading

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
1 files    0 ❌

Results for commit d85246c.

♻️ This comment has been updated with latest results.

netbe
netbe reviewed Nov 13, 2024
View reviewed changes
WireAPI/Package.resolved
@@ -0,0 +1,42 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: do we need to add this one, as we always use WireAPI within monorepo Xcode workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add it to .gitignore.
It's just automatically generated when we open the module using Xcode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add it to .gitignore?

WireAPI/Sources/WireAPI/APIs/AccountsAPI/Responses/UpgradeToTeamResponseEnvelopeV7.swift Outdated Show resolved Hide resolved
WireAPI/Sources/WireAPI/Components/ResponseParser.swift Outdated Show resolved Hide resolved
WireAPI/Sources/WireAPI/Models/Accounts/UpgradeAccountEnvelope.swift Outdated Show resolved Hide resolved
WireDomain/Sources/WireDomain/UseCases/IndividualToTeamMigrationUseCaseImplementation.swift

public struct IndividualToTeamMigrationUseCase {
private let accountsAPI: AccountsAPI
private let logger: WireLogger = WireLogger(tag: "individual-to-team-migration")
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer a static one like WireLogger.individualToTeam

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?

WireDomain/Sources/WireDomain/UseCases/IndividualToTeamMigrationUseCaseImplementation.swift Outdated Show resolved Hide resolved
wire-ios-sync-engine/Source/SessionManager/SessionFactories.swift
@@ -42,6 +68,12 @@ open class AuthenticatedSessionFactory {
reachability: Reachability,
minTLSVersion: String?
) {
self.apiService = APIService(
backendURL: environment.backendURL,
// TODO: Use the authentication storage from https://github.com/wireapp/wire-ios/pull/2084
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

wire-ios-sync-engine/Source/SessionManager/SessionFactories.swift
import WireDataModel


final class InMemoryAuthenticationStorage: AuthenticationStorage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand this is temporary too until renew access token PR is merged, right?
otherwise I would suggest to move to own file

Copy link
Collaborator

Choose a reason for hiding this comment

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

There already is an InMemoryAuthenticationStorage so feel free to make it public and use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's exactly the same code, just copy-pasted.
And it is temporary until the renew access token PR is merged.

@johnxnguyen Do we want the InMemoryAuthentication to be and remain public?

WireAPI/Sources/WireAPI/Models/Network/TLSVersion.swift
@@ -30,6 +30,23 @@ public enum TLSVersion {

case v1_3

public static func minVersionFrom(_ string: String?) -> TLSVersion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure to see how this is related to this PR, is it for something further

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The APIService needs a WireAPI.TLSVersion.
The AuthenticatedSessionFactory has one, but as a String.
The WireAPI.TLSVersion had no way of being instantiated using a String.

So Monday I asked if I should port the WireTransport.TLSVersion init logic to WireAPI.TLSVersion, and was told I indeed should.

WireAPI/Sources/WireAPI/APIs/AccountsAPI/Responses/UpgradeToTeamResponseEnvelopeV7.swift Outdated Show resolved Hide resolved
johnxnguyen
johnxnguyen reviewed Nov 13, 2024
View reviewed changes
Copy link
Collaborator

@johnxnguyen johnxnguyen left a comment

Choose a reason for hiding this comment

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

Looks good, I'll wait for the conversations to be resolved before approving.

WireAPI/Sources/WireAPI/APIs/AccountsAPI/AccountsAPIBuilder.swift Outdated Show resolved Hide resolved
WireAPI/Sources/WireAPI/APIs/AccountsAPI/AccountsAPIError.swift Outdated Show resolved Hide resolved
WireAPI/Sources/WireAPI/Models/Accounts/UpgradeAccountEnvelope.swift Outdated Show resolved Hide resolved
WireAPI/Sources/WireAPI/Models/Accounts/UpgradeAccountEnvelope.swift Outdated Show resolved Hide resolved
WireAPI/Sources/WireAPI/Models/Accounts/UpgradeAccountEnvelope.swift Outdated Show resolved Hide resolved
wire-ios-sync-engine/Source/SessionManager/SessionFactories.swift
import WireDataModel


final class InMemoryAuthenticationStorage: AuthenticationStorage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There already is an InMemoryAuthenticationStorage so feel free to make it public and use it here.

@El-Fitz El-Fitz force-pushed the feat/individual-to-team-migration-ui branch from 12300f5 to 7ebdca4 Compare November 15, 2024 09:09
@El-Fitz El-Fitz force-pushed the feat/invidiual-to-team-migration-api branch from aa88e45 to d85246c Compare November 15, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnxnguyen johnxnguyen johnxnguyen left review comments

@samwyndham samwyndham samwyndham left review comments

@netbe netbe netbe left review comments

@caldrian caldrian Awaiting requested review from caldrian

Assignees

@El-Fitz El-Fitz

Labels
echoes/initiative: personal-users-to-team-users...
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@El-Fitz @netbe @samwyndham @johnxnguyen