initial commit to the repo

[averissimo]

Pull Request

Fixes https://github.com/insightsengineering/coredev-tasks/issues/499

Notable features and information

• Initial support for ssh connector
• Requires:
  • App developer to assign path and corresponding dataname
• Supports
  • Any type of read file function (defaults to read.csv without arguments, but any other can be used)
  • If hostname is not defined by App Developer, user can define it
  • Context-aware error messages (different for connection issues and remote file download)

Sample code

Note that you may need to:

• Setup SSH server (openssh) locally or have access to a remote environment.
• Have valid csv files at those endpoints to test a successful connector

library(teal)
pkgload::load_all("../teal.connectors.ssh")
x <- ssh_connector(
  host = "localhost",
  paths = list(ADSL = "/invalid_path.csv", ADTTE = "/valid.csv", ADRS = "/another_invalid.csv")
)

app <- init(
  data = x,
  modules = list(example_module())
)
shiny::runApp(app)

Screenshots

[kartikeyakirar]

@averissimo In Rstudio I did not get build tab when I pull the branch. do you get this option ?

Edits: its working.

[kartikeyakirar]

@averissimo I have added functionality for custom error message in the Harbour connector. It would be great to have it here, suggesting why data access failed. Maybe we could directly show invalidation on input widgets as well.

Also I was thinking to enhance UI with better title and container positioning.

[github-actions[bot]]

🧪 Test coverage: 0.00%

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  ---------
R/ssh_connector.R      216     216  0.00%    72-351
R/utils.R               42      42  0.00%    3-49
TOTAL                  258     258  0.00%

Results for commit: 4057a8ecfb050fd0687095c9e5d28f8c2bbd74cc

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[averissimo]

@kartikeyakirar thanks for the error handling suggestions :heart: . I think it's a lot nicer for the user this way.

Note:

• Reverted the ssh_connector() to include shiny code
  • This way teal_data executes all code inside and shows reproducible code
• ssh_connector() no longer has id parameter
• Added your suggestion of "SSH connection info" that shows after a successful connection
  • Only available if there's a problem with files
  • On invalid credentials, it doesn't show it though
  • It requires extra failure handling complexity and the error message is sufficient

Screenshots

### Invalid credentials  ### Invalid path  ### Multiple invalid 

[kartikeyakirar]

@averissimo The code looks great and works as expected.

Thoughts:

• I think we should make displaying SSH connection info optional, based on a flag. If it's not too much trouble, we can implement this, or we can wait to see if the user requests it.

• Just an observation: the connection info doesn't need to be collapsible, as once it's uncollapsed, we can't collapse it back. It's not an issue, but I wanted to point it out as it might be unnecessary.

• When we have lots of files, it takes a lot of time to download, and if the first file fails, it continues to read the rest of the files. This could be time-consuming and might delay showing an error, which ultimately means data can't be accessed. However, this feature is useful for debugging which file is unavailable. I believe keeping this feature is more beneficial.

Please check the following:

• The tests need to be updated as they are currently failing due to the recent updates.

• Run devtools::check() and urlchecker::url_check().

[averissimo]

@kartikeyakirar

Thoughts:

* I think we should make displaying SSH connection info optional, based on a flag. If it's not too much trouble, we can implement this, or we can wait to see if the user requests it.

* Just an observation: the connection info doesn't need to be collapsible, as once it's uncollapsed, we can't collapse it back. It's not an issue, but I wanted to point it out as it might be unnecessary.

I don't think the information in that table is meaningful as it only contains data when the connection is successful, hence collapsing by default.

It just reinforces that it was successful, which might be helpful for the user once there's an error with data.

* When we have lots of files, it takes a lot of time to download, and if the first file fails, it continues to read the rest of the files. This could be time-consuming and might delay showing an error, which ultimately means data can't be accessed. However, this feature is useful for debugging which file is unavailable. I believe keeping this feature is more beneficial.

I got a suggestion for a workaround that's fast :-) https://github.com/insightsengineering/teal.connectors.ssh/pull/1/commits/4f80d23296b1becdec050cf6bbee1369cafa3ff1

It fallbacks to check if file exist instead of downloading

I'm fixing the test, thanks for catching that. Some of the URLs will fail until this repo is public.

[averissimo]

Test are corrected and it now allows multi-line expression as part of read_expression argument.

With a very clean code. Documentation was updated

library(teal)
x <- ssh_connector(
  host = "localhost",
  paths = list(ADSL = "/valid.csv", ADTTE = "/valid.csv"),
  read_expression = expression(
    read_custom <- function() read.csv(file = path, header = TRUE),
    read_custom()
  )
)

app <- init(
  data = x,
  modules = list(example_module())
)
if (interactive()) {
  shiny::runApp(app)
}