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.

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

[spaceship] Disable media scaling when a trailer is uploaded #14032

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tutipeti
Copy link
Contributor

@tutipeti tutipeti commented Jan 7, 2019

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

With the fix from #14028 uploading trailers works, now saving the version with the trailer failed for me. After some try and error I figured out that the issue is that fastlane still tries to turn on scaling for devices even if there is a trailer uploaded (and no screenshot). This fixes #14030

Description

I changed the code for the initial data setup, so that devices that have trailers are also disabling scaling. I also made sure to disable scaling at the end of uploading a new trailer.
I tested this by trying to upload the same video again, and now I could finally save it.

Note: I found several tests for the trailer-related code, but all of it is commented out. Should I add tests for my fix/try to get the existing tests working?

@janpio
Copy link
Member

janpio commented Jan 8, 2019

Working tests would indeed be awesome 🥇 👍

@joshdholtz
Copy link
Member

@tutipeti If you could add tests that would be 💯 I will also try this out soon here too and try to get this out in the next release

@tutipeti
Copy link
Contributor Author

@joshdholtz I'll try to get back to this next week, was away for a week of vacation. Have a nice weekend :)

@tutipeti
Copy link
Contributor Author

@joshdholtz I looked into the tests again, but I kinda got stuck at stubbing the requests.

Can any of you guys give me a hint of how that's supposed to be done? Do you have an example app on App Store Connect that I can use to upload a video, and then just copy the request/response from Charles?

@tutipeti
Copy link
Contributor Author

Paging @joshdholtz 😩

@@ -480,7 +481,7 @@ def upload_geojson!(geojson_path)
end

# Uploads or removes a screenshot
# @param icon_path (String): The path to the screenshot. Use nil to remove it
Copy link
Member

Choose a reason for hiding this comment

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

Uh, that name was always broken?

@janpio
Copy link
Member

janpio commented Feb 25, 2019

I am not sure what @joshdholtz expected really, but best take a look at the other tests.
We usually work with out own Apple Developer accounts, there is no shared one we use to generate the requests or similar.

@janpio janpio closed this Feb 25, 2019
@janpio
Copy link
Member

janpio commented Feb 25, 2019

Ups, that should just have been a simple comment. Sorry @tutipeti 🙈

@janpio janpio reopened this Feb 25, 2019
@pedrodelavega
Copy link

This is an important fix - without it I can't upload app previews...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[spaceship] Uploading first app trailer fails while saving version
7 participants