episphere / quest

experimenting with the idea of a questionnaire markup
https://episphere.github.io/quest/
2 stars 11 forks source link

Issue with renderer & displayif statements #358

Closed boyd-mj closed 2 years ago

boyd-mj commented 3 years ago

@FrogGirl1123, @danielruss, @iqbal-singh, @Davinkjohnson We are encountering issues with displayif statements not functioning as expected in some cases. All of the cases we've encountered so far have involved displayif statements that reference variables from other modules. This is happening when the previous results option is used and added to JSON on the testing site. We initially encountered this when testing the ALCLIFE grids, which use AGE (all questions display regardless of what's entered), but have since encountered it with some Module 4 questions that rely on WORK & WORK6 among others. In some cases, no code changes have been made since we last tested so we wondering if this is an issue with the previous results function of the rendered, or possibly Quest?

I'm out tomorrow, but @joshid-ims may be able to provide more details if needed in my absence.

joshid-ims commented 3 years ago

Gwen tried the displayif issue with WORK and WORK6 in a separate Quest window. It worked as expected. That means the json memory was not getting cleared properly by Clear Memory button.

Grid displayif is still an issue though.

danielruss commented 3 years ago

@joshid-ims @boyd-mj Can you please give me and simple example of when it fails. Also we may need to make sure the PWA is passing the results from the previous module to quest. Please check with @naiyume

danielruss commented 3 years ago

image

Davinkjohnson commented 3 years ago

@danielruss is your last comment meant to demonstrate the displayif functionality? I'm not sure if it's that or you're getting at something else...

Based on @boyd-mj 's comment it looks like the displayif for grids is the concern here.

boyd-mj commented 3 years ago

Based on @boyd-mj 's comment it looks like the displayif for grids is the concern here.

Yes, though we still experience issues with other displayifs. It's just that in those cases, the issue can be resolved by our tester opening a new browser window. We have only seen this when referencing data from another module as mentioned.

danielruss commented 3 years ago

yes, I was showing that if the data is passed in appropriate. Displayif's work. I'll look for a grid example.

danielruss commented 3 years ago

😱 the displayif in ACLIFE4 in module 3

danielruss commented 3 years ago

@iqbal-singh take a look at Module 3 [ALCOHOL4?]. The response does not appear to be written localforage. There is the question has an input[type="number"] and ID. I expected to see

ALCOHOL4 -> {ID: num} but I see undefined.

danielruss commented 3 years ago

@joshid-ims @boyd-mj This is really weird. And should be fixed in the next version of quest. The white space is causing trouble.

[ALCOHOL3?]
How old were you when you <b>first</b> drank alcohol?

Age first drank alcohol |__|__|min=0 max=isDefined(AGE,age)|< ->  ALCOHOL4>

Notice that the < -> ALCOHOL4> is on the same line as the |__|__|min=0 max=isDefined(AGE,age)|. If you add an enter (newline) so that the < -> ALCOHOL4> is on it's own line, I get a value for ALCOHOL4. Let me know if this resolves your problem.

danielruss commented 3 years ago

While I'm here... #NR should go to SECTION4 not INTROSE

[ALCOHOL2?] How many alcoholic beverages have you had in your <b>entire life</b>?
(0) 10 or less -> SECTION4
(1) 11—49 -> SECTION4
(2) 50—99 -> ALCOHOL3
(3) 100 or more -> ALCOHOL3
< #NR -> INTROSE>
joshid-ims commented 3 years ago

I have made these changes and we will be testing it soon.

joshid-ims commented 3 years ago

I added line returns in ALCOHOL3, ACOHOL6 and still it is showing all age ranges. It does show correct age range in ALCLIFE2 though.

danielruss commented 3 years ago

@joshid-ims @boyd-mj Did you every see the grids rows working? I never implemented a display if on the grid rows. Maybe Hui implemented them? If not, this becomes a high priority item.

At some point we need to discuss how to re-implement this without so many displayif's. The idea was that displayif would be a last choice. Not used on every question. I see why you did it. We need to discuss with Montse about quest training so that the logic of questionnaires are easier to convert.

Also, if you ever have logic that gets really crazy like or(or(or(or.... let me know. We can may you life easier.

joshid-ims commented 3 years ago

We will test a few more grids with displayif's and let you know.

There are a places where there are crazy or(or(or... s also.

joshid-ims commented 3 years ago

GRID_SECSMLF2 also shows all age ranges irrespective of the age entered.

boyd-mj commented 3 years ago

@danielruss we did not encounter this issue when we tested grids before the prior Module 3 release and the code has not changed, which is what prompted us to think it might be an issue with Quest or the renderer.

danielruss commented 3 years ago

So at some point the displayif's worked. I'll dig deeper.

danielruss commented 3 years ago

@joshid-ims @boyd-mj @iqbal-singh I looked at the code that builds the grids. The relevant part of the code has not changed in 13 months. Do you have a sec for a quick teams call/webex?

joshid-ims commented 3 years ago

sure

danielruss commented 3 years ago

tried calling on teams. No response

joshid-ims commented 3 years ago

I tried to send msg on teams but looks like external msgs are blocked for us. Can you send invite instead?

danielruss commented 3 years ago

ok webex invite sent

boyd-mj commented 3 years ago

I won't be able to join, but Deepti can update our team. Thanks!

danielruss commented 3 years ago

I think I fixed the first issue here.. grids with displayif were not working. The second issue is if you use information from another module, it cannot be used with display if. working on that next

danielruss commented 3 years ago

actually it may work completely. waiting for @iqbal-singh

danielruss commented 3 years ago

also fixed the problem where the previousResults where not being considered (for exists, doesNotExist, noneExists, valueEquals, valueIsOneOf).

joshid-ims commented 3 years ago

Thanks. We will test on Monday.

danielruss commented 3 years ago

@joshid-ims ... waiting on @iqbal-singh Nothing has been checked into production yet.

danielruss commented 3 years ago

@joshid-ims, @iqbal-singh finished his review. Ready for testing

joshid-ims commented 3 years ago

Thanks. We will test this.

danielruss commented 2 years ago

stale issue... closing...