Port send jslib to mobile #1219
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
MGibson1
force-pushed
the
port-send-jslib-to-mobile
branch
3 times, most recently
from
a30ce2d to
e77ca14
Compare
kspearrin
reviewed
Jan 14, 2021
Took the testing infrastructure from bitwarden/server
These are mostly happy-path tests to ensure a reasonably correct implementation
MGibson1
force-pushed
the
port-send-jslib-to-mobile
branch
from
e77ca14 to
07a2258
Compare
MGibson1
commented
Jan 15, 2021
cscharf
reviewed
Jan 15, 2021
MGibson1
commented
Jan 15, 2021
src/Core/Services/SendService.cs
Outdated
| response = await _apiService.PostSendFileAsync(fd); | ||
| break; | ||
| default: | ||
| response = new SendResponse(); |
There was a problem hiding this comment.
This default behavior idea might just be a bad one. It's identifying a possible future bug and recovering from it in a hard-to-track-down manner.
I think throwing would be better. Thoughts?
There was a problem hiding this comment.
I ended up adding the throw only here.
I don't think we want to throw whenever we encounter one since that would do bad things like lock up Sync if clients get out of date relative to servers.
Instead, we should just keep the client from ruining saved data by complaining last minute that it doesn't know what it's doing.
I think it's best to only throw on unknown send types here. I don't think we want to throw whenever we encounter one since that would do bad things like lock up Sync if clients get out of date relative to servers. Instead, keep the client from ruining saved data by complaining last minute that it doesn't know what it's doing.
cscharf
approved these changes
Jan 15, 2021
vvolkgang
pushed a commit
that referenced
this pull request
Jun 20, 2024
vvolkgang
pushed a commit
that referenced
this pull request
Jun 20, 2024
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.
Overview
Port of Send PRs to jslib (bitwarden/jslib#190, bitwarden/jslib#192, bitwarden/jslib#205, bitwarden/jslib#242). This PR does not implement any actual use of the SendService.
Testing
Projects
Common
This project is a copy from the bitwarden/server/Core.Test project, with some additions. Given the similarity here, we should think about creating a new repo for common testing items and accessing them through a submodule.
Core.Test
This is a test bed for the Send feature I'm implementing. Crypto tests are copied from jslib tests. Send model and service tests are happy path tests to ensure proper function since I'm not implementing anything that uses them.
Automated Testing
An Automated test runner is added to the
androidjob. Unfortunately,dotnetis not capable of building theandroidandiOSprojects, so it is run against only theCore.Testproject. We'll have to add another line per test project we add.Files Changes/Added
Core.Testproject to the android job pipeline.test/Common.csprojandtest/Core.TestTesting Files Changes
Common
Core.Test