Fix #786 - Show error message when invalid project id is used for Central

[mayank8318]

Closes #786

What has been done to verify that this works as intended?

Manually tested with correct and incorrect project IDs as shown below:
https://www.youtube.com/watch?v=Rd0Vuyg1PMg

Why is this the best possible solution? Were any other approaches considered?

I have tried to keep it as consistent with the current design as possible.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This change will warn the user of an invalid project ID for the Central server at the time of setting up the connection. No risks to the users.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

No

[codecov-io]

Codecov Report

Merging #787 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #787      +/-   ##
============================================
- Coverage     48.37%   48.34%   -0.04%     
+ Complexity     1631     1630       -1     
============================================
  Files           192      192              
  Lines         10268    10275       +7     
  Branches        737      739       +2     
============================================
  Hits           4967     4967              
- Misses         4950     4958       +8     
+ Partials        351      350       -1

Impacted Files	Coverage Δ	Complexity Δ
...takit/briefcase/reused/transfer/CentralServer.java	80.98% <0%> (+0.56%)	36 <0> (ø)	⬇️
...sed/transfer/sourcetarget/CentralServerDialog.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...eused/transfer/sourcetarget/target/PushTarget.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...eused/transfer/sourcetarget/source/PullSource.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...g/opendatakit/briefcase/ui/export/ExportPanel.java	48.51% <0%> (-1.99%)	14% <0%> (-1%)
...g/opendatakit/briefcase/reused/UncheckedFiles.java	46.21% <0%> (+1.68%)	32% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd1b966...423c6a4. Read the comment docs.

[ggalmazor]

Hi, @mayank8318!

I think your solution solves the issue but it introduces some duplication that I think we could avoid.

The new tests defined in PullSource and PushTarget feel confusing at this moment because there's a credentials tests and then a project test, which requiers a session token, which has been already done and tested in the credentials test.

I think I'd expect the test to follow this steps:

• Get the session token
  • If this fails, return the session token response
• Use the session token to run the project id check and return the response

This would mean that we wouldn't need a server.getCredentialsTestRequest() anymore.

On the other hand, in CentralServerDialog, do you think we could be more precise when managing the responses? I'm not sure if Central will respond with a specific 404.X status code when asking for an inexisting project, but maybe we could use that to differentiate between not finding the server and not finding the project.

[mayank8318]

@ggalmazor As of now, Central responds with 404.1(inside the response body) for status not found.

Also, I tried with random wrong server URL(valid tho) and noticed that the same message is shown as the status code is the same.

[mayank8318]

If it is an invalid server then it essentially means that the test to get the token has to fail with a 404 and not a 401 (as for invalid credentials). We can leverage this fact in the new test in PullSource and PushTarget and return a special two-layer Response object back to CentralServerDialog?
@ggalmazor

[ggalmazor]

If it is an invalid server then it essentially means that the test to get the token has to fail with a 404 and not a 401 (as for invalid credentials). We can leverage this fact in the new test in PullSource and PushTarget and return a special two-layer Response object back to CentralServerDialog?

That's a good idea, but I think it would complicate stuff for Aggregate. I don't think it's worth the hassle.

Now that you've confirmed this, I think our best option would be to change that error message from "Project ID does not exist" to "Central server or project not found". What do you think?

[mayank8318]

@ggalmazor Yes, that makes sense. Updating the PR now

[ggalmazor]

LGTM!

[ggalmazor]

Hi @mayank8318! Could you rebase master and deal with conflicts?

[mayank8318]

@ggalmazor Done

[kkrawczyk123]

Testes with success!!
Verified on Ubuntu, MacOS and Windows.

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"