-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
fix(feedback): last minute tweaks to apis to align implementation and docs #4873
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4873 +/- ##
=============================================
- Coverage 92.257% 92.193% -0.064%
=============================================
Files 659 659
Lines 77310 77527 +217
Branches 27933 27281 -652
=============================================
+ Hits 71324 71475 +151
- Misses 5890 5960 +70
+ Partials 96 92 -4
... and 33 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7fb7afb | 1230.12 ms | 1251.04 ms | 20.92 ms |
b8dd0fc | 1268.96 ms | 1271.90 ms | 2.94 ms |
2b19b82 | 1237.45 ms | 1256.60 ms | 19.16 ms |
c76f9b0 | 1249.78 ms | 1265.14 ms | 15.36 ms |
e8b2fb4 | 1230.70 ms | 1242.84 ms | 12.14 ms |
bee3e88 | 1232.65 ms | 1247.33 ms | 14.68 ms |
2862f33 | 1208.83 ms | 1227.29 ms | 18.45 ms |
5d6ce0e | 1206.72 ms | 1228.67 ms | 21.95 ms |
aa27ac0 | 1214.02 ms | 1227.42 ms | 13.40 ms |
eae2b59 | 1229.62 ms | 1251.83 ms | 22.21 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7fb7afb | 20.76 KiB | 419.70 KiB | 398.94 KiB |
b8dd0fc | 20.76 KiB | 401.39 KiB | 380.63 KiB |
2b19b82 | 21.58 KiB | 542.19 KiB | 520.61 KiB |
c76f9b0 | 22.85 KiB | 406.69 KiB | 383.84 KiB |
e8b2fb4 | 20.76 KiB | 427.23 KiB | 406.46 KiB |
bee3e88 | 22.30 KiB | 749.95 KiB | 727.64 KiB |
2862f33 | 22.31 KiB | 775.17 KiB | 752.86 KiB |
5d6ce0e | 22.85 KiB | 405.38 KiB | 382.53 KiB |
aa27ac0 | 20.76 KiB | 432.21 KiB | 411.45 KiB |
eae2b59 | 21.58 KiB | 705.31 KiB | 683.72 KiB |
Previous results on branch: armcknight/feat(feedback)/final-fixes
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
5baa4a5 | 1227.49 ms | 1245.43 ms | 17.94 ms |
f09e892 | 1220.22 ms | 1238.80 ms | 18.57 ms |
cc14e49 | 1209.31 ms | 1231.54 ms | 22.24 ms |
38ae188 | 1221.04 ms | 1246.91 ms | 25.87 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
5baa4a5 | 22.31 KiB | 821.57 KiB | 799.26 KiB |
f09e892 | 22.31 KiB | 820.98 KiB | 798.67 KiB |
cc14e49 | 22.31 KiB | 821.57 KiB | 799.26 KiB |
38ae188 | 22.31 KiB | 820.94 KiB | 798.63 KiB |
Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackFormViewModel.swift
Show resolved
Hide resolved
Sources/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackFormConfiguration.swift
Show resolved
Hide resolved
...ces/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackThemeConfiguration.swift
Show resolved
Hide resolved
...ces/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackThemeConfiguration.swift
Show resolved
Hide resolved
...ces/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackThemeConfiguration.swift
Show resolved
Hide resolved
...ces/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackThemeConfiguration.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it 50% of the PR, only 1 question
Sources/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackConfiguration.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments, but other than that LGTM
} | ||
dict["source"] = source.rawValue | ||
dict["source"] = source.description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
: See my comment in #4874
let sut = SentryFeedback(message: "Test feedback message", name: "Test feedback provider", email: "[email protected]", source: .custom, attachments: [Data()]) | ||
|
||
let serialization = sut.serialize() | ||
XCTAssertEqual(try XCTUnwrap(serialization["message"] as? String), "Test feedback message") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
: see my comment in #4784
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a few suggestions.
@@ -248,7 +246,7 @@ extension UserFeedbackUITests { | |||
try assertHookMarkersNotExist() | |||
|
|||
widgetButton.tap() | |||
|
|||
XCTAssert(sendButton.waitForExistence(timeout: 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: Any reason why we can't use waitForExistence
func waitForExistence(_ message: String) { | |
XCTAssertTrue(self.waitForExistence(timeout: TimeInterval(10)), message) | |
} |
@@ -617,7 +617,7 @@ - (void)captureFeedback:(SentryFeedback *)feedback withScope:(SentryScope *)scop | |||
alwaysAttachStacktrace:NO]; | |||
SentryTraceContext *traceContext = [self getTraceStateWithEvent:preparedEvent withScope:scope]; | |||
NSArray *attachments = [[self attachmentsForEvent:preparedEvent scope:scope] | |||
arrayByAddingObjectsFromArray:[feedback attachments]]; | |||
arrayByAddingObjectsFromArray:[feedback attachmentsForEnvelope]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: If this is a bug, it would be valuable to add a test for this or to assert for the correct attachments in an existing test to avoid having the same issue in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it was due to the rename I mentioned in #4873 (comment). There are a couple test cases that check at least for its presence:
sentry-cocoa/Tests/SentryTests/Integrations/Feedback/SentryFeedbackTests.swift
Lines 31 to 90 in cd24266
func testSerializeWithAllFields() throws { | |
let sut = SentryFeedback(message: "Test feedback message", name: "Test feedback provider", email: "[email protected]", screenshot: Data()) | |
let serialization = sut.serialize() | |
XCTAssertEqual(try XCTUnwrap(serialization["message"] as? String), "Test feedback message") | |
XCTAssertEqual(try XCTUnwrap(serialization["name"] as? String), "Test feedback provider") | |
XCTAssertEqual(try XCTUnwrap(serialization["contact_email"] as? String), "[email protected]") | |
XCTAssertEqual(try XCTUnwrap(serialization["source"] as? String), "widget") | |
let attachments = sut.attachments() | |
XCTAssertEqual(attachments.count, 1) | |
XCTAssertEqual(try XCTUnwrap(attachments.first).filename, "screenshot.png") | |
XCTAssertEqual(try XCTUnwrap(attachments.first).contentType, "application/png") | |
} | |
func testSerializeWithNoOptionalFields() throws { | |
let sut = SentryFeedback(message: "Test feedback message", name: nil, email: nil) | |
let serialization = sut.serialize() | |
XCTAssertEqual(try XCTUnwrap(serialization["message"] as? String), "Test feedback message") | |
XCTAssertNil(serialization["name"]) | |
XCTAssertNil(serialization["contact_email"]) | |
XCTAssertEqual(try XCTUnwrap(serialization["source"] as? String), "widget") | |
let attachments = sut.attachments() | |
XCTAssertEqual(attachments.count, 0) | |
} | |
private let inputCombinations: [FeedbackTestCase] = [ | |
// base case: don't require name or email, don't input a name or email, don't input a message or screenshot | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: nil, messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
// set a screenshot | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: nil, messageInput: nil, includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
// set a message | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: nil, messageInput: "Test message", includeScreenshot: false), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback with no name or email address with message: Test message."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: nil, messageInput: "Test message", includeScreenshot: true), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback with no name or email address including attached screenshot with message: Test message."), | |
// set an email address | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: "[email protected]", messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: "[email protected]", messageInput: nil, includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: "[email protected]", messageInput: "Test message", includeScreenshot: false), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback with no name at [email protected] with message: Test message."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: nil, emailInput: "[email protected]", messageInput: "Test message", includeScreenshot: true), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback with no name at [email protected] including attached screenshot with message: Test message."), | |
// set a name | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: nil, messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: nil, messageInput: nil, includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: nil, messageInput: "Test message", includeScreenshot: false), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback for tester with no email address with message: Test message."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: nil, messageInput: "Test message", includeScreenshot: true), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback for tester with no email address including attached screenshot with message: Test message."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: "[email protected]", messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: "[email protected]", messageInput: nil, includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: "[email protected]", messageInput: "Test message", includeScreenshot: false), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback for tester at [email protected] with message: Test message."), | |
(config: (requiresName: false, requiresEmail: false, nameInput: "tester", emailInput: "[email protected]", messageInput: "Test message", includeScreenshot: true), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback for tester at [email protected] including attached screenshot with message: Test message."), | |
// require email address | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: nil, messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following fields: email and description."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: nil, messageInput: nil, includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following fields: email and description."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: nil, messageInput: "Test message", includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: email."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: nil, messageInput: "Test message", includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: email."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: "[email protected]", messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: "[email protected]", messageInput: nil, includeScreenshot: true), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following field: description."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: "[email protected]", messageInput: "Test message", includeScreenshot: false), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback with no name at [email protected] with message: Test message."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: nil, emailInput: "[email protected]", messageInput: "Test message", includeScreenshot: true), shouldValidate: true, expectedSubmitButtonAccessibilityHint: "Will submit feedback with no name at [email protected] including attached screenshot with message: Test message."), | |
(config: (requiresName: false, requiresEmail: true, nameInput: "tester", emailInput: nil, messageInput: nil, includeScreenshot: false), shouldValidate: false, expectedSubmitButtonAccessibilityHint: "You must provide all required information before submitting. Please check the following fields: email and description."), |
I added a ui test to check the envelope contents for the attachments in 7678e18
Sources/Swift/Integrations/UserFeedback/Configuration/SentryUserFeedbackConfiguration.swift
Show resolved
Hide resolved
* Sets the email and name field text content to `SentryUser.email` and `SentryUser.name`. | ||
* - note: Default: `true` | ||
*/ | ||
public var useSentryUser: Bool = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: What about useScopeUser? I mean it's not the Sentry user but the user form the scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is there ever a user object besides what's stored in the scope? For reference, here's the doc for this: https://github.com/getsentry/sentry-docs/pull/11191/files#diff-d02ef3c10131140fde283da64efeebe4d40068d0fbb2ebc610be2377571b7ad4R89
I don't want to change the name because this is what it's called in other SDKs. But I can improve the headerdoc here to indicate it comes from the current scope's user.
Fix a couple things I noticed while finalizing the docs PR: getsentry/sentry-docs#11191 and would like to fix before officially releasing the feature for the first time
#skip-changelog