[NAVAND-830] payment methods [DO-NOT-MERGE]#1449
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1449 +/- ##
============================================
- Coverage 76.90% 75.76% -1.15%
+ Complexity 937 883 -54
============================================
Files 129 125 -4
Lines 4019 3911 -108
Branches 582 578 -4
============================================
- Hits 3091 2963 -128
- Misses 678 686 +8
- Partials 250 262 +12
|
| } | ||
|
|
||
| /** | ||
| * Retention policy for the various payment methods. |
There was a problem hiding this comment.
Why do we call it a retention policy? It thought it means something different in the context of JVM 🤔
There was a problem hiding this comment.
I think it's legacy from the previous version, that we copy-and-paste whenever Defs are used. Feel free to change it
|
@LukasPaczos , @RingerJK , @dzinad , the PR isn't ready to be merged but it's ready for review 🙂 |
| @Test | ||
| public void emptyPaymentMethodsList() { | ||
| RouteOptions routeOptions = routeOptions().toBuilder() | ||
| .paymentMethodsList(Collections.<String>emptyList()) |
There was a problem hiding this comment.
Is there a test that checks what happens if you pass null (if it's supported) or don't call the method at all?
There was a problem hiding this comment.
Nope. I just felt confidence in case of null, because it's a default value. I didn't felt confidence in case of an empty list and added the test 🙂
If you think it's important, I will add a test for the null case 👍
There was a problem hiding this comment.
What will happen? Will there be a parameter with an empty string or no parameter at all? Since it's no parameter at all for an empty list I guess it will be the same for null so here I don't think it's necessary. I usually add tests for null and empty as a rule, just to be sure. No pressure tho. :)
| * @return this builder | ||
| */ | ||
| @NonNull | ||
| public abstract Builder paymentMethods(@Nullable String paymentMethods); |
There was a problem hiding this comment.
We're missing updates to MapboxDirections that use this new parameter (and corresponding request/response tests).
There was a problem hiding this comment.
Added. But I'm not sure about response tests in the services-directions module. They seem like a duplication of tests from the services-dirctions-model module, as they just check how a JSON test file transforms to model. WDYT?
dzinad
force-pushed
the
vv-payment-options
branch
2 times, most recently
from
eda159f to
aa31cbb
Compare
|
Fixed the conflicts, but the feature is temporarily removed from backend. Will place the ticket in "to track" and wait for the updates (targeting next week). |
dzinad
force-pushed
the
vv-payment-options
branch
from
aa31cbb to
d2e5736
Compare
LukasPaczos
changed the title
dzinad
force-pushed
the
vv-payment-options
branch
2 times, most recently
from
bf17a46 to
aec6240
Compare
|
Tested it on staging. Works fine, I've adjusted some string constants. Leaving the DO NOT MERGE label until it's in production. |
dzinad
force-pushed
the
vv-payment-options
branch
from
aec6240 to
04c4723
Compare
LukasPaczos
left a comment
LukasPaczos
left a comment
There was a problem hiding this comment.
LGTM but let's double-check the lane data validity and docs before merging, as discussed internally.
dzinad
force-pushed
the
vv-payment-options
branch
from
04c4723 to
526e833
Compare
4 participants
No description provided.