fix_: ignore log partial API request #21312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (4)
|
Parveshdhull
approved these changes
Sep 24, 2024
ilmotta
reviewed
Sep 24, 2024
There was a problem hiding this comment.
@qfrank, shouldn't this mobile PR use all the new *V2 endpoints you created in the status-go PR? Otherwise, doing QA in this mobile PR won't validate the new endpoints work.
Another way to frame my question is, why did we create V2 functions if status-mobile is the only consumer of the functions in mobile/status.go? Why keep V1 and V2 versions if we only need to point to the V2 ones?
If this mobile PR changed to use the new V2 endpoints:
- We would QA only once, thus no need for a follow-up.
- We would be able to remove all deprecated functions in
mobile/status.go.
Maybe I'm missing something and there's a good reason to keep deprecated and v2 functions in mobile/status.go.
It depends on time, I thought we should fix the issue quickly, WDYT?🤔 cc @churik We can create separate PR to use all the new *V2 endpoints and validating then.
create V2 should keep application stable, status-mobile is NOT the only consumer of the functions in mobile/status.go, desktop also use it. |
qfrank
force-pushed
the
fix/LogVerifyDatabasePassword
branch
from
6f08f86 to
bc9f76d
Compare
Thanks for the clarification. I think we could at least use the V2 endpoints on the mobile repo in this PR. It thought it was just a matter of renaming in mobile in a handful of places because V2 functions are backwards compatible. But this is just my personal preference, if you and QA team think it's alright to have a follow-up no worries by me. |
ilmotta
approved these changes
Sep 25, 2024
It's easy for you but not for me. Merely renaming and constructing JSON request parameters. I doubt that it will be completed in a short time for me under strict clojure coding rules 😉. If someone(clojure expert) can help, that's another story |
No worries then @qfrank. To each their own cup of tea 😄 PR more than approved, it's just that QAs should be aware they won't be testing some of the new code being merged in status-go, as that will come only in the follow-up PR using new endpoints. I would like to help, but realistically I'll probably just cause a delay on the process due to other tasks on me already. |
|
@qfrank @ilmotta From my side I don't have an info where particular methods can be used in the app, so I checked:
I didn't notice any sensitive info (i.e. passwords) in Let me know if additional check are required. |
yes
no more from my side, thank you! |
86% of end-end tests have passedFailed tests (7)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletMultipleDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
|
57% of end-end tests have passedFailed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Passed tests (4)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
e2e test failures are not related, green light, thanks! |
Closed
qfrank
force-pushed
the
fix/LogVerifyDatabasePassword
branch
from
bc9f76d to
bcc8f05
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For change description see details
relate comment1 / comment2
status: ready.