Implement Sync Beta Group Commands #147

DechengMa · 2020-05-18T05:32:44Z

Implement basic sync beta group command.

Delete local group files -> request to remove groups in server
Create local group files -> request create groups in server
After create -> pull back and sync local
Edit local group files -> update server groups
Wrap up and restructure codes
Apply new file system on it
Refactor file structure and remove duplicate data

📝 Summary of Changes

Changes proposed in this pull request:

Add let id: String? field to Beta Group model. I know we don't want to do this but it's a must coz two groups can have the same name.
Add var testers: [String] to Beta Group for storing tester emails
Make BetaGroup extend to Equatable, Hashable for comparing in Set, to SyncResultRenderable for rendering dry run and sync result, to FileNameGettable for generating a beatified filename (a String extension was made for snakeCased filename).
Create SyncResultRenderer for printing sync and dry run result
Create enum SyncStrategy for both rendering result and handle the update.
Create protocol ResourceFolderManagerand BetaGroupFolderManager, for managing folders and group files.
Tweak ListBetaTestersOperation and create UpdateBetaGroupOperation. Create service function pullBetaGroups, updateBetaGroup, deleteBetaGroup.
Implement SyncBetaGroupsCommand which contains PullBetaGroupsCommand and PushBetaGroupsCommand.

⚠️ Items of Note

When edit file to change group, only particular field are editable: for a beta group, only name, public link, link limit, link enabled can be changed (according to API), and if others are changed, when push it will have no effect. So this means the user must have some instructions.

🧐🗒 Reviewer Notes

💁 Example

Usage:

asc testflight betagroup sync pull [--output-path <output-path>]
asc testflight betagroup sync push [--input-path <input-path>] [--dry-run]

--output-path and --input-path is a folder path.

CLI will go through all the files in that folder and find file with a suffix of yml and read it as a beta group file.

Sample Output:

TODO

Sample file structure

TODO

🔨 How To Test

asc testflight betagroup sync pull

asc testflight betagroup sync push --dry-run

asc testflight betagroup sync push

How to

Note: Please run `pull` first to get server groups and testers before run push

Create a new group via sync:

Create a file, name it whatever you like in folder ./config/betagroups(or the input folder path), suffix must be yml
Put some basic group info in it, eg

app:
  bundleId: com.example.foo // MUST HAVE
groupName: testgroup // MUST HAVE, MUST UNIQUE (please don't create group with the same name in same app)
publicLinkEnabled: false
publicLinkLimit: 5
publicLinkLimitEnabled: false

Run command asc testflight betagroup sync push

Delete a group

Delete the group file in folder
Run command asc testflight betagroup sync push

Update a group

Directly edit the group file in the config folder
fields that support editing: (edit other fields will not work)

groupName
publicLinkEnabled
publicLinkLimit
publicLinkLimitEnabled

Run command asc testflight betagroup sync push

Sources/AppStoreConnectCLI/Model/BetaGroup.swift

Sources/AppStoreConnectCLI/Services/Operations/ListBetaTestersOperation.swift

adamjcampbell

Some preliminary suggestions and thoughts
Looking pretty good! This may go through some designing though

Sources/AppStoreConnectCLI/Files/BetaGroupFileManager.swift

Sources/AppStoreConnectCLI/Files/ResourceFolderManager.swift

Sources/AppStoreConnectCLI/Helpers/String+Helpers.swift

Sources/AppStoreConnectCLI/Model/App.swift

adamjcampbell · 2020-05-21T04:33:16Z

Sources/AppStoreConnectCLI/Model/BetaGroup.swift

@@ -5,7 +5,8 @@ import Combine
 import Foundation
 import SwiftyTextTable

-struct BetaGroup: TableInfoProvider, ResultRenderable, Equatable {
+struct BetaGroup: TableInfoProvider, ResultRenderable {
+    let id: String?


Same goes for BetaGroup

An optional id is for creating a beta group. When creating a new group we don't have the id yet. we can basically create a file with just minimal information (eg. group name, link enabled, etc) and push back

adamjcampbell · 2020-05-21T04:34:33Z

Sources/AppStoreConnectCLI/Model/BetaGroup.swift

@@ -14,6 +15,7 @@ struct BetaGroup: TableInfoProvider, ResultRenderable, Equatable {
    let publicLinkLimit: Int?
    let publicLinkLimitEnabled: Bool?
    let creationDate: String?
+    var testers: String? // CSV file path


This is where things can get interesting.. Having variables that will populated when pulled from disk but not populated when retrieved from the network

adamjcampbell · 2020-05-21T04:37:09Z

Sources/AppStoreConnectCLI/Model/BetaGroup.swift

+    static func == (lhs: BetaGroup, rhs: BetaGroup) -> Bool {
+        return lhs.id == rhs.id &&
+            lhs.groupName == rhs.groupName &&
+            lhs.publicLinkEnabled == rhs.publicLinkEnabled &&
+            lhs.publicLinkLimit == rhs.publicLinkLimit &&
+            lhs.publicLinkLimitEnabled == rhs.publicLinkLimitEnabled
+    }
+
+    func hash(into hasher: inout Hasher) {
+        hasher.combine(id)
+        hasher.combine(groupName)
+        hasher.combine(publicLinkEnabled)
+        hasher.combine(publicLinkLimit)
+        hasher.combine(publicLinkLimitEnabled)
+    }


We could potentially only use the id for matching.. do we ever foresee having duplicates?

Did you have to override the default implementations because of the testers file path? There may also be a different solution here 🤔

Very true. for now, this is only for comparing the BetaGroup object difference, not for testers. when doing something like Set(localGroups).subtracting(serverGroups) I can get the difference and update the server by local

Sources/AppStoreConnectCLI/Model/BetaGroup.swift

Sources/AppStoreConnectCLI/Services/Operations/UpdateBetaGroupOperation.swift

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PushBetaGroupsCommand.swift

Changes were made upon request

orj · 2020-06-22T00:51:34Z

@DechengMa sorry I've not reviewed this. I'd like to try building a workflow based on this work to test it out.

DechengMa · 2020-06-22T01:00:14Z

@DechengMa sorry I've not reviewed this. I'd like to try building a workflow based on this work to test it out.

Sure no problem. Please PM directly if there's anything that I can do to help. And I'll be around for fixing any bugs / unexpected behaviours in the first place.

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PullBetaGroupsCommand.swift

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/SyncBetaGroupsCommand.swift

orj · 2020-06-24T05:42:34Z

Sources/FileSystem/Beta Group/BetaGroupProcessor.swift

+
+extension BetaGroup: FileProvider {
+    var fileName: String {
+        "\(app.bundleId ?? "")_\(groupName).yml"


I think we should be normalising the filename a bit better here. groupName could be an unsafe filename. We should turn all non alpha numeric characters into underscores.

extension String { func filenameSafe() -> String { let unsafeFilenameCharacters = CharacterSet(charactersIn: " *?:/\\.") return str.components(separatedBy: unsafeFilenameCharacters).joined(separator: "_") } }

I'm working with an app that has a beta group named "a/h_Security" so this code creates a subdirectory named "bundleId_a" with a file in it called "h_Security.yml"

For an account that has a lot of groups it would be better to nest them in subdirectories. The app.bundleId could be a subdirectory rather than a prefix.

Also, surely the app.bundleId will be non-nil? When will it ever be nil?

Actually thinking about he normalised/safe filename some more I think when group names are translated to file/path names they should be really cleaned up:

Eg: A group name like d_App Team (Learn & Adopt) should be translated to d-app-team-learn-adopt. The problem with that however is that removing characters might mean there could be clashes. We might have to come up with some resolution strategy in that case.

What I'm trying to suggest here is that the filenames should be names that a developer might choose if they were authoring them themselves.

orj

This is good work @DechengMa but I think we need to refine it a bit.

The current pulled structure is very flat and includes redundant data. I think we should change the structure so it is like this:

config/betagroups/
- com.example.appid/
  - safe-group-name/
    - betagroup.yml
    - betatesters.csv

orj · 2020-06-24T06:01:21Z

Something else I'd like us to clean up here is the duplication of information. When we pull down the details of the beta group the app element is repeated in each betagroup file. This isn't ideal. It would be better if it was not repeated.

DechengMa · 2020-07-10T06:05:05Z

a new redesigned sync command will be implemented in #203

DechengMa added the enhancement New feature or request label May 18, 2020

DechengMa self-assigned this May 18, 2020

DechengMa changed the title ~~Testflight/Sync/ Implement PullBetaGroupsCommand~~ Implement Sync Beta Group Commands May 19, 2020

DechengMa force-pushed the testflight/sync-betagroup branch from 8e881e4 to e822f13 Compare May 20, 2020 08:06

DechengMa commented May 20, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Model/BetaGroup.swift Outdated Show resolved Hide resolved

DechengMa commented May 20, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Services/Operations/ListBetaTestersOperation.swift Outdated Show resolved Hide resolved

DechengMa marked this pull request as ready for review May 20, 2020 23:54

DechengMa requested review from adamjcampbell, orj and n-a-f-i-s-a May 21, 2020 00:07

adamjcampbell previously requested changes May 21, 2020

View reviewed changes

DechengMa commented May 21, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PushBetaGroupsCommand.swift Outdated Show resolved Hide resolved

DechengMa force-pushed the testflight/sync-betagroup branch from e822f13 to 986827a Compare May 22, 2020 01:31

DechengMa requested a review from adamjcampbell May 25, 2020 23:38

DechengMa marked this pull request as draft June 11, 2020 07:44

DechengMa force-pushed the testflight/sync-betagroup branch from 986827a to fe92a6c Compare June 12, 2020 04:23

DechengMa marked this pull request as ready for review June 12, 2020 04:38

DechengMa mentioned this pull request Jun 16, 2020

Implement sync testers in sync beta group command #163

Closed

orj reviewed Jun 24, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PullBetaGroupsCommand.swift Outdated Show resolved Hide resolved

orj reviewed Jun 24, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PullBetaGroupsCommand.swift Outdated Show resolved Hide resolved

orj reviewed Jun 24, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/SyncBetaGroupsCommand.swift Outdated Show resolved Hide resolved

orj reviewed Jun 24, 2020

View reviewed changes

Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/SyncBetaGroupsCommand.swift Outdated Show resolved Hide resolved

orj reviewed Jun 24, 2020

View reviewed changes

orj requested changes Jun 24, 2020

View reviewed changes

DechengMa added 2 commits July 1, 2020 09:49

Point SDK source back to AvdLee Master

7e9c933

update fileContent() -> Data to return FileContent

73ce2cc

DechengMa added 19 commits July 1, 2020 09:49

Add id to beta group, make group name non-nil

a9b95c7

Add delete to processor with some tweaks, create betagroup processor

9e564bd

Remove not found error in ListBetaTesterOperation. make options optional

5226dfc

Create service function pullBetaGroups

265826f

Implement basic pull beta groups command

701f95e

Create BetaTesterProcessor for rendering CSV

4688745

Wire BetaGroupProcessor up with TesterProcessor

077f789

Model.BetaGroup + Hashable, Equatable

4b74c09

Add read() to BetaGroupProcessor

94e03c8

Implement UpdateBetaGroupOperation, add update and delete service func

1c50987

Create SyncStrategy, SyncResultRenderable, SyncResultRenderer

ede1d0f

Wire up PushBetaGroupCommand with sync funcs and strategies

b7d3cce

Reformat files for linting get passed

bc1f122

Introduce SyncResourceComparator,

49e4ecb

Apply new SyncResourceComparator to sync betagroup command

aa393dd

Add tests to SyncResourceComparator

4495c84

PushBetaGroupsCommand syncBetaGroup function code separation

1679e6f

mark compareIdentity non-optional in ResourceComparable, tweak sync test

cb7cc95

Reformat Pull betagroup command desc, remove default command for sync

0e381c0

DechengMa force-pushed the testflight/sync-betagroup branch from 46aa03a to 0e381c0 Compare July 1, 2020 00:10

DechengMa marked this pull request as draft July 2, 2020 04:23

DechengMa closed this Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Sync Beta Group Commands #147

Implement Sync Beta Group Commands #147

DechengMa commented May 18, 2020 •

edited

Loading

adamjcampbell left a comment

adamjcampbell May 21, 2020

DechengMa May 22, 2020

adamjcampbell May 21, 2020

adamjcampbell May 21, 2020

DechengMa May 21, 2020 •

edited

Loading

orj commented Jun 22, 2020

DechengMa commented Jun 22, 2020

orj Jun 24, 2020 •

edited

Loading

orj Jun 24, 2020

orj Jun 24, 2020

orj Jun 24, 2020

orj left a comment •

edited

Loading

orj commented Jun 24, 2020

DechengMa commented Jul 10, 2020

Implement Sync Beta Group Commands #147

Implement Sync Beta Group Commands #147

Conversation

DechengMa commented May 18, 2020 • edited Loading

📝 Summary of Changes

⚠️ Items of Note

🧐🗒 Reviewer Notes

💁 Example

Usage:

Sample Output:

TODO

Sample file structure

TODO

🔨 How To Test

How to

Note: Please run pull first to get server groups and testers before run push

Create a new group via sync:

Delete a group

Update a group

adamjcampbell left a comment

Choose a reason for hiding this comment

adamjcampbell May 21, 2020

Choose a reason for hiding this comment

DechengMa May 22, 2020

Choose a reason for hiding this comment

adamjcampbell May 21, 2020

Choose a reason for hiding this comment

adamjcampbell May 21, 2020

Choose a reason for hiding this comment

DechengMa May 21, 2020 • edited Loading

Choose a reason for hiding this comment

orj commented Jun 22, 2020

DechengMa commented Jun 22, 2020

orj Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

orj Jun 24, 2020

Choose a reason for hiding this comment

orj Jun 24, 2020

Choose a reason for hiding this comment

orj Jun 24, 2020

Choose a reason for hiding this comment

orj left a comment • edited Loading

Choose a reason for hiding this comment

orj commented Jun 24, 2020

DechengMa commented Jul 10, 2020

DechengMa commented May 18, 2020 •

edited

Loading

Note: Please run `pull` first to get server groups and testers before run push

DechengMa May 21, 2020 •

edited

Loading

orj Jun 24, 2020 •

edited

Loading

orj left a comment •

edited

Loading