Survey with conditional getting stuck in a loop

[sagesteppe]

Bug description

Hey -first off sorry getting I'm caught up with the new GH issues thing.

Hey! Loving the package so far but I keep coming across a recurring snag.

I use one conditional question to dispatch users to one of 12 sets of questions. Each of these sets are essentially identical. At the end of these 4 questions, users from each of the 12 sets should be able to go to the 'end' screen and exit survey. However usually when I take the test survey, I get caught in a loop where instead of going to 'end' or a later question in the parallel series I go back to the first question in the set.

I've made a reprex which will do a similar thing below. However, the formatting in these boxes has some issues, so I've put everything up into a repo so you can take a look or clone the results - Sorry the formatting is pretty abysmal now.

git@github.com:sagesteppe/ReprexSurvey.git
https://github.com/sagesteppe/ReprexSurvey.git

Any idea what the issue is? Typically the survey will go to the last question in a section, in this case the one about Ringo, before bouncing back to the start of the section.

Steps to reproduce

git@github.com:sagesteppe/ReprexSurvey.git

Run app, and after clicking through the live survey you should realize you are in a loop.

Expected behavior

Should be able to exit the survey through the 'end'

Actual behavior

loops

Your environment

sessionInfo()
R version 4.4.2 (2024-10-31)
Platform: x86_64-pc-linux-gnu
Running under: Ubuntu 22.04.5 LTS

Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so.3
LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblasp-r0.3.20.so; LAPACK version 3.10.0

locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8 LC_MONETARY=en_US.UTF-8
[6] LC_MESSAGES=en_US.UTF-8 LC_PAPER=en_US.UTF-8 LC_NAME=C LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C

time zone: America/Chicago
tzcode source: system (glibc)

attached base packages:
[1] stats graphics grDevices utils datasets methods base

loaded via a namespace (and not attached):
[1] compiler_4.4.2 tools_4.4.2 rstudioapi_0.17.1

RStudio.Version()
....

$mode
[1] "desktop"

$version
[1] ‘2024.9.1.394’

$long_version
[1] "2024.09.1+394"

$release_name
[1] "Cranberry Hibiscus"

[jhelvy]

Thanks I'll take a look at it. I might even suggest this approach of making a separate repo with an example survey in it for people to post issues because it makes it a lot easier to debug. With two files (survey.qmd and app.R), it's hard to fully describe the issue.

[jhelvy]

Okay, I can confirm there is a bug here. I heavily simplified your example to narrow it down to the issue at hand: reprex-jph.zip

I've also broken down my response here into sections. This is lengthy, but it's a bit complicated so it requires a lot of explanation.

Understanding the issue

In my simplified demo, I have 4 pages: page1, page2, page3, and end.

On page1 you have a question that determines which page to skip to: page2 or page3. If you go to page3, the next button sends you to the end page as expected - no issue.

But if on page1 you choose page2, the next button sends you to page2 (as expected), but then the next button on page3 sends you back to page2, and you enter an endless loop. This is because of the skip logic in the app.R server:

input$question1 == 'page2' ~ "page2"

This logic evaluates as TRUE if you chose page2 on the question on page1, and because of that the next button on page3 sends you to page2.

Where the problem is occurring is in the handle_skip_logic() function in the server.R file. At the end of that function, we have the following line:

if (condition_result & (current_page_id != rule$target)) {
    return(rule$target)
}

This handles two conditions, both of which must be TRUE for the user to be sent to the target skip page.

The first condition is the evaluated result of the sd_skip_if() conditions provided in the app.R server. In this example, this is the input$question1 == 'page2' line, which will always be TRUE once the user selects page2 on the question on page1.

The second condition checks that we're not already on the target page, which prevents us from getting stuck on the target page forever. This is why you are allowed to go on to page3 from page2, because when you're on page2, the second condition (current_page_id != rule$target) is FALSE, so the skip target is ignored and you go on to page3. But then once you get to page3, both of these conditions are now TRUE again, so the user gets sent to skip target, which is page2. And now we're in a loop.

Searching for a solution

Restricting skipping to only skip forward

One idea is that we could restrict the skip logic such that if the skip target is already past, it gets ignored, which means we could only use the skip logic to skip forward in the survey. This is pretty easy to implement. We know the order of the page ids, so we could add an additional check to see if rule$target comes before current_page_id, in which case we could ignore the skip logic.

This would solve this immediate example, but it might be overly restrictive. For example, what if we had a survey where on page3 you have a question called question_change that ask a yes/no multiple choice question if you want to go back to page2 to change your answer for something on that page? Then you'd have something like this in the sd_skip_if():

input$question_change == 'yes' ~ "page2"

But here the skip target (page2) comes before the current page (page3), so the logic would be ignored. But this could be addressed if we allowed the user to specify the direction of skipping: "forward" or "backward". I see two ways to do this:

1) Split sd_skip_if() into two functions: sd_skip_forward() & sd_skip_backward()

If we took the approach I described above to restrict skipping to only be forward, then we could still allow users to skip backwards by eliminating sd_skip_if() and replacing it with sd_skip_forward() and sd_skip_backward(). The functions would behave just like sd_skip_if(), except they would have the added restrictions on the direction of the skipping. This would allow you to use sd_skip_forward() to solve the immediate example described in this issue, but it would also allow you to use sd_skip_backward() to implement the other example I described where we would want to allow users to be sent to previous pages in the survey.

This approach would introduce a rather serious breaking change in how skipping works. We would have to depreciate sd_skip_if() and make it perform the same as sd_skip_forward() where only forward skipping is allowed, with a message printed that clarifies this behavior and directs users to start using sd_skip_forward() and sd_skip_backward().

2) Add some kind of direction argument to sd_skip_if()

I'm not sure how to do this, but it would require some modification to how skipping logic is defined. Right now we have this:

input$question1 == 'page2' ~ "page2"

So somewhere in here we'd have to add "forward" or "backward". I don't like this, because I really like this current API of using the ~ symbol. You only have to remember two things: condition and target. If someone has a better idea though, this could be an option.

[jhelvy]

@pingfan-hu what do you think? I'm leaning towards breaking up sd_skip_if() into sd_skip_forward() and sd_skip_backward().

[pingfan-hu]

Yes professor, I tried the repo of @sagesteppe and did find out the problem, and yes, it's because of we are trying to go back to a previous page, which violates the conditions handle_skip_logic().
I think it'll definitely be better if we can take care of both forward and backward conditions, but I'm leaning to keeping the current sd_skip_if() function, but add an argument of maybe direction to it, default to "forward", and can be set to "backward".

I have 2 reasons for that:

1. Forward skipping covers more than a half of the cases, so keeping the function name simple and default to "forward" will take care of the majority.
2. We have current users who've constructed their surveys using the existing sd_skip_if() function. They don't want their surveys to break.

So our solution can both embrace a new feature but keep the old instances safe.

[jhelvy]

I'm still leaning towards having two separate functions, mostly because I like the condition ~ target API for specifying the skip conditions. I just don't know where a direction argument could go. If we want to keep the condition ~ target API, then we are limited to having just one thing on the left and one thing on the right. So we might have to do something like this:

input$question1 == 'page2' ~ list(target = "page2", direction = "backward")

We could use list() or just a named vector with c(), but in any case I think we'd have to use some structure like that.

For most cases, the default direction would be "forward", so maybe for those cases we don't specify anything at all. With that idea, maybe we could do something like this:

input$question1 == 'page2' ~ c(target = "page2", backwards = TRUE)

In this case the default would be backwards = FALSE, in which case you wouldn't have to do all this. But it's still rather ugly.

In contrast, if we change sd_skip_if() to sd_skip_forward(), then all we have to do is depreciate sd_skip_if(), and in the depreciation setting we just make sd_skip_if() become a wrapper around the new sd_skip_forward() function. That would preserve the functionality of sd_skip_if() for prior users - if you use sd_skip_if() it will still work just the same, except it will be restricted to only skipping forward. Then if users want to skip backwards, they could use sd_skip_backward(). I feel like this is a cleaner set up.

[pingfan-hu]

Yup it's better to separate them, and I forgot we could do depreciation.
Sure, let's split them.

[jhelvy]

Okay let's table this for a future version. It's important, but we have a lot of changes to include already in the next version. Once we get that one merged, we can update this.