ittybittyapps / appstoreconnect-cli

An easy to use command-line tool for interacting with the Apple AppStore Connect API
MIT License
172 stars 17 forks source link

Process TestflightProgramDifference #233

Closed adamjcampbell closed 3 years ago

adamjcampbell commented 3 years ago

📝 Summary of Changes

Changes proposed in this pull request:

⚠️ Items of Note

This is just a part of getting dry run for sync up and running. The command is still unimplemented and would be nice to make a testing set to gain confidence in how it operates.

🧐🗒 Reviewer Notes

💁 Example

$ swift run asc testflight sync push
Beta group named: testGroup2 will be added to app with bundleId: com.example
Beta Tester with email: example@gmail.com will be added to groups: testGroup2
Error: This command has not been implemented

🔨 How To Test

Add beta groups / beta testers to existing apps after using asc testflight sync pull to see if this behaves correctly

DechengMa commented 3 years ago

@adamjcampbell Just found another edge case: when adding a tester to beta-testers.csv, but not add it to any group: It will say Beta Tester with email: foo@gmail.com will be added to groups: with an empty group list. Do we want to show will be added to app: bundleId or just skip in this case?

adamjcampbell commented 3 years ago

Just found another edge case: when adding a tester to beta-testers.csv, but not add it to any group

I'm a bit torn on this one. I'm thinking that it's not actually valid to do this. It should be added to at least one group when we read from file. So I think I'll throw an error in this case and state that it needs to be added to a group.

I should probably also error out in the case that it's added to a group but not the main testers file. What do you think?

DechengMa commented 3 years ago

Just found another edge case: when adding a tester to beta-testers.csv, but not add it to any group

I'm a bit torn on this one. I'm thinking that it's not actually valid to do this. It should be added to at least one group when we read from file. So I think I'll throw an error in this case and state that it needs to be added to a group.

I should probably also error out in the case that it's added to a group but not the main testers file. What do you think?

Can throw an error as a reminder for both cases, I agree, sounds reasonable.

adamjcampbell commented 3 years ago

@DechengMa added an error for the no beta groups instance:

Example:

$ asc testflight sync push
Error: Tester with email: bob@jones.com being added to apps: iba.test2 has not been added to any beta groups
DechengMa commented 3 years ago

@DechengMa added an error for the no beta groups instance:

Example:

$ asc testflight sync push
Error: Tester with email: bob@jones.com being added to apps: iba.test2 has not been added to any beta groups

Sorry I found this is not quite feasible. In my test account, I already have some existing testers in an App in server that has no been assign to any beta groups. If I'd like to perform a deleteTesterfromApp, this will not allow me to do. 😢 Any thoughts?

adamjcampbell commented 3 years ago

Sorry I found this is not quite feasible. In my test account, I already have some existing testers in an App in server that has no been assign to any beta groups. If I'd like to perform a deleteTesterfromApp, this will not allow me to do. 😢 Any thoughts?

In this case should you not delete those testers from the beta-testers.csv file?

DechengMa commented 3 years ago

Sorry I found this is not quite feasible. In my test account, I already have some existing testers in an App in server that has no been assign to any beta groups. If I'd like to perform a deleteTesterfromApp, this will not allow me to do. 😢 Any thoughts?

In this case should you not delete those testers from the beta-testers.csv file?

If I delete those, in next sync pull, I guess they will come back again. 🤔

adamjcampbell commented 3 years ago

If I delete those, in next sync pull, I guess they will come back again. 🤔

I guess we do need to implement add to app and remove from app then not just groups

adamjcampbell commented 3 years ago

@DechengMa pushed some changes covering more add and remove app cases.

This all behaves fine when you pull down the entire remote configuration but I think we'll need to pay attention to having the correct behaviour if you are only meant to change the apps which you have pulled down (with a filter) and not others

DechengMa commented 3 years ago

@DechengMa pushed some changes covering more add and remove app cases.

This all behaves fine when you pull down the entire remote configuration but I think we'll need to pay attention to having the correct behaviour if you are only meant to change the apps which you have pulled down (with a filter) and not others

I have tested these code and it works nice. Can merge when you happy 😃 . I also have another minor suggestion you might want to consider: If we can have error been thrown when a tester is added to a group, but test is not existing in that app, that will be 💯 eg.

id: 123456-ae73-421c-ad07-123456
groupName: New Group 8
testers:
- example@google.com

and the example@google.com doesn't exist in beta-testers.csv. Currently, this will produce no result.

adamjcampbell commented 3 years ago

@DechengMa I added that error you were looking for: It should print out in the format: Error: Tester with email: foo@bar.com in beta group named: groupName for app: com.example.app is not included in the beta-testers.csv file