Bugfix FXIOS-12033 [SEC] Fix check for {searchTerms} in base URL
#26223
Conversation
Client.app: Coverage: 34.93
Generated by 🚫 Danger Swift against 33d2adc |
mattreaganmozilla
force-pushed
the
mr/12033
branch
2 times, most recently
from
9e5ad84 to
df0c1de
Compare
mattreaganmozilla
force-pushed
the
mr/12033
branch
from
df0c1de to
33d2adc
Compare
There was a problem hiding this comment.
thanks @mattreaganmozilla! mainly nits and some comments to clarify since I haven't been too close in this area.
| @@ -46,11 +46,13 @@ struct ASSearchEngineUtilities { | |||
| return URLQueryItem(name: $0.name, value: value) | |||
| } | |||
| // From API docs: "This may be skipped if `{searchTerm}` is included in the base." | |||
| if let searchArg = searchURL.searchTermParamName, !searchURL.base.contains("{searchTerm}") { | |||
| // Note: term vs terms is not a typo. | |||
| // Note: there is a typo in the docs, the value is searchTerms (plural). | |||
There was a problem hiding this comment.
nit: should we reference the docs? not sure where the typo was. Or we can add a JIRA ticket number here with more context.
Also interesting to see that the previous comment says:
// Note: term vs terms is not a typo. Do you have context on this 🤔 or seems like this was a mistake?
There was a problem hiding this comment.
The short version is that I was going by the docs we had and that was the origin of the first comment. But I double-checked the data eventually (and then checked with AS team) and it turned out the docs were wrong. Those headers are being fixed, but I wanted to be sure to document which version (searchTerm vs searchTerms) was correct.
| @@ -10,6 +10,59 @@ import WebKit | |||
| import MozillaAppServices | |||
|
|
|||
| class ASSearchEngineUtilitiesTests: XCTestCase { | |||
| private let leo_eng_deu_engine_no_search_params = | |||
There was a problem hiding this comment.
🔥 thanks for adding tests!
| let expected = "https://dict.leo.org/englisch-deutsch/{searchTerms}" | ||
| XCTAssertEqual(result, expected) |
There was a problem hiding this comment.
curious, how come we include searchTerms if we don't have search params?
There was a problem hiding this comment.
The additional parameters are for things besides the search parameters themselves (they're for additional arguments like the client or partner code identifier, etc.). So it's valid to have a search URL without them.
📜 Tickets
Jira ticket
Github issue
💡 Description
Fixes an issue with checks for
{searchTerm}in the base URL of our AS-based search engines. It turns out the API headers did, in fact, contain a typo.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)