Refactor prototype_build_details_comment for Firebase App Distribution
#630
Conversation
…stribution App Center will be discontinued and we are migrating to Firebase App Distribution, so this action needs to derive its metadata from the `lane_context` exposed by the `firebase_app_distribution` action instead of the `appcenter_upload` one now. Besides, less parameters are now needed, which allowed us to simplify the implementation and make it more readable.
With some help from Cursor's Composer and Claude
Generated by 🚫 Danger |
Co-authored-by: Claude <claude-3.5-sonnet@cursor.com>
Add extensive test coverage for the `prototype_build_details_comment` action, including: - Error handling for invalid URLs - HTML escaping for app names and metadata - Metadata handling for edge cases - App icon handling for different input types We also reverted the escaping of metadata keys and values that we introduced previously, because we want to allow the caller to use HTML for those after all. Co-authored-by: Claude <claude-3.5-sonnet@cursor.com>
In particular, don't default to `:firebase:` icon if no icon was provided but a non-FAD `download_url` was provided for direct download instead.
Results of testing it in real projects(After pointing the test repo's On an iOS project using Firebase
On an Android project which uploads its
|
| <table> | ||
| <tr> | ||
| <td rowspan='#{metadata_rows.count + 1}' width='260px'><img src='#{qr_code_url}' width='250' height='250' /></td> | ||
| <td><b>App Name</b></td><td>#{icon_img_tag} #{app_display_name}</td> | ||
| <td><b>App Name</b></td><td>#{CGI.escape_html(app_display_name)}</td> |
| + 'If you uploaded the build to App Center, we recommend leaving this nil (the comment will use the URL to the App Center build for the QR code)', | ||
| description: <<~DESC, | ||
| The URL to use to download/install the build. | ||
| - If you used `firebase_app_distribution` to upload the build during the same `fastlane` run, you should leave this nil |
There was a problem hiding this comment.
Do you think it would be worth it to validate whether there's only firebase_app_distribution or download_url (+ test for that scenario)? 🤔
There was a problem hiding this comment.
The way I've implemented it is to still purposely allow for both, not only because I think it shouldn't be an error and crash if there are both (eg a build we upload to FAD and S2 for some reason) but also because I can imagine actually wanting to have both an Installation URL (FAD lane_context) and a Direct Download URL—e.g. imagine Tumblr Android build being sent both to FAB and to Closed Testing track with an invite link…
Realistically I don't think we will ever really have the need to ever pass both in any project we currently have in practice tbh. But I also don't see a reason why to forcibly forbid it.
There was a problem hiding this comment.
(That's also why I used "should" in this help doc and not "must", and why in install_links I didn't use an elsif but instead allowed for both if paths (download_url and FirebaseReleaseInfo) to be taken, to not assume it'll always be one or the other)
There was a problem hiding this comment.
Got it! Yes, makes sense 👍
🚀
AliSoftware
deleted the
prototype_build_details_comment/appcenter-to-firebase-app-distribution
branch
What does it do?
Given App Center's imminent deprecation and shutdown, we are migrating our Prototype Builds from App Center to Firebase App Distribution.
This PR:
prototype_build_details_commentaction to rely on the data exposed fromfirebase_app_distributionaction instead of the ones from theappcenter_uploaduploadInternal References
Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.