Allow non-standard datanames in filter data

[averissimo]

Pull Request

Fixes insightsengineering/teal#1366

Related:

• Allow non-standard datanames in teal .raw_data teal#1382
• Allow non-standard datanames in filter data #622
• Allow non-standard datanames in code dependency teal.data#340

Changes description

• Removed assertion on datanames that start with alphabetic character
• Fix problem with JS namespace in filter panel
• Fix crash when filtering using MAE (both SE and Matrix)
• [x ] Fix upload of snapshot file that is not compatible
• Ignore datanames that contain functions, language, expression (and other non-data objects)
  • [Bug]: functions in data raise warnings teal#1352

[github-actions]

Unit Tests Summary

  1 files   29 suites   23s ⏱️
367 tests 367 ✅ 0 💤 0 ❌
819 runs  819 ✅ 0 💤 0 ❌

Results for commit 5a6c823.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
utils	💀	$0.04$	$-0.04$	check_simple_name_behaves_as_expected
utils	👶		$+0.00$	should_allow_for_integer_input
utils	👶		$+0.00$	should_replace_UTF_characters_outside_the_allowed_range
utils	👶		$+0.00$	should_replace_all_escape_characters_from_JQuery_selectors
utils	👶		$+0.00$	should_replace_all_quotes_characters_with___
utils	👶		$+0.00$	should_replace_dots_with___when_id_is_otherwise_valid
utils	👶		$+0.00$	should_replace_non_ASCII_characters_in_middle_of_id_with___
utils	👶		$+0.01$	should_replace_non_ASCII_characters_in_the_start_end_of_id_with___
utils	👶		$+0.00$	should_take_vector_input

Results for commit 7aeaf16

♻️ This comment has been updated with latest results.

[github-actions]

Code Coverage Summary

Filename                        Stmts    Miss  Cover    Missing
----------------------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------
R/calls_combine_by.R                7       0  100.00%
R/choices_labeled.R                49      14  71.43%   25, 36, 41, 51-56, 68, 72-76
R/count_labels.R                  102       0  100.00%
R/filter_panel_api.R               27       1  96.30%   132
R/FilteredData-utils.R             68      25  63.24%   21-24, 27-30, 52-57, 153, 175-184
R/FilteredData.R                  539     186  65.49%   110, 184, 323, 395, 500-508, 531, 550-593, 613-616, 657, 694, 715-747, 770-772, 775-789, 793-803, 806-849, 906, 918-940
R/FilteredDataset-utils.R          23       1  95.65%   125
R/FilteredDataset.R               232      72  68.97%   48, 147, 179, 206-276, 379
R/FilteredDatasetDataframe.R      120       8  93.33%   87, 148, 158, 233-237
R/FilteredDatasetDefault.R         18       4  77.78%   104-117
R/FilteredDatasetMAE.R            133      37  72.18%   56, 117-122, 159-164, 168-169, 187-209
R/FilterPanelAPI.R                 10       0  100.00%
R/FilterState-utils.R             101       2  98.02%   264, 294
R/FilterState.R                   363      61  83.20%   89, 213, 231-235, 242-243, 257-258, 264-265, 321, 323, 325, 377, 420, 641, 684-707, 717-736, 771-777, 786-792
R/FilterStateChoices.R            352     109  69.03%   301, 368-375, 379-396, 423-426, 439-450, 462-470, 474-503, 523-526, 529-532, 542-567, 578, 583, 594
R/FilterStateDate.R               213     127  40.38%   230, 282-437
R/FilterStateDatettime.R          307     197  35.83%   266, 318-547
R/FilterStateEmpty.R               53      31  41.51%   89, 99-104, 117, 129-169
R/FilterStateExpr.R                83      68  18.07%   167-280
R/FilterStateLogical.R            194     142  26.80%   136, 158, 218, 222-404
R/FilterStateRange.R              410     104  74.63%   262, 384, 517-521, 524-534, 537, 548-554, 565-577, 581-591, 595-597, 610-636, 651, 654, 668-685, 720-725, 735-737
R/FilterStates-utils.R             70       7  90.00%   108, 127, 188-194
R/FilterStates.R                  370      32  91.35%   63, 80-84, 196, 318-327, 412-415, 458, 496, 559-563, 608, 728-731
R/FilterStatesDF.R                  5       0  100.00%
R/FilterStatesMAE.R                10       1  90.00%   40
R/FilterStatesMatrix.R              7       0  100.00%
R/FilterStatesSE.R                218      62  71.56%   36, 73-75, 85-87, 110-117, 125-132, 208, 233, 252-270, 278-296, 304
R/include_css_js.R                  5       5  0.00%    12-16
R/teal_slice.R                    107       4  96.26%   131, 195-196, 206
R/teal_slices.R                    84       5  94.05%   150-155
R/test_utils.R                     21       0  100.00%
R/utils.R                          24       0  100.00%
R/variable_types.R                 15       1  93.33%   48
R/zzz.R                            17      17  0.00%    3-47
TOTAL                            4357    1323  69.64%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  --------
R/FilteredData.R             -1       0  -0.06%
R/FilterStates.R             +9      +1  -0.06%
R/FilterStatesMatrix.R       +4       0  +100.00%
R/FilterStatesSE.R           +2       0  +0.26%
R/utils.R                    +4       0  +100.00%
TOTAL                       +18      +1  +0.10%

Results for commit: 5a6c823

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[gogonzo]

Exotic datanames don't fail but the filter-panel for them is not displayed. There is a problem with namespace id's which are based on dataname. Specifying initial teal_slices fails an app.

app

pkgload::load_all("teal.data")
pkgload::load_all("teal.slice")
pkgload::load_all("teal")

app <- teal::init(
  data = teal_data() |> within({
    `iris raw` <- iris
    `%iris%` <- iris
  }), 
  # filter = teal_slices(
  #   teal_slice("iris raw", "Species", selected = "setosa")
  # ),
  modules = modules(
    example_module("exotic"),
    example_module("exotic-specified", datanames = "iris raw")
  )
)


runApp(app)

[averissimo]

Exotic datanames don't fail but the filter-panel for them is not displayed. There is a problem with namespace id's which are based on dataname. Specifying initial teal_slices fails an app.

One way to solve that problem globally for teal.slice is to replace the NS and moduleServer that are used to create the Shiny's namespace.

The proposal in 23c2bcf would add minimal code to the existing codebase.

WDYT @gogonzo ?

[averissimo]

An alternative to this proposal would be to identify call in the codebase that generates an id, and encode that id manually.

This would add a lot of noise as well as being prone to errors.

note: The proposal needs some extra tests to ensure all dataname types are working. As of now I only tested with data.frame-based dataname.

[gogonzo]

One way to solve that problem globally for teal.slice is to replace the NS and moduleServer that are used to create the Shiny's namespace.

I like the approach, not sure yet if we should make NS/moduleServer or NS2/moduleServer2 just to avoid a confusion.

An alternative to this proposal would be to identify call in the codebase that generates an id, and encode that id manually.

There is a lot to cover and a lot to miss. I prefer to make own NS/moduleServer

[averissimo]

I like the approach, not sure yet if we should make NS/moduleServer or NS2/moduleServer2 just to avoid a confusion.

Both have drawbacks,

\1. NS/moduleServer is not completely transparent to developers as I would expect those to be imported from Shiny

\2. NS2/moduleServer2 is more explicit, but involves a bunch of find/replace as well as it might be easy for us to not use them when fixing some bug or adding new features

We could mitigate 2. by overriding NS/moduleServer in teal.slice with a stop error saying it should not be used.

I tend towards 1. as it's the cleaner solution and it has a very stable API. In the end we're just forwarding with modified id

[gogonzo]

Wonderful! 👍 (please fix the tests and let's move on)