fix: js-evaluator escape characters

[chrismclarke]

PR Checklist

• [x] PR title descriptive (can be used in release notes)

Description

• Fix JSEvaluator to handle javascript escape characters (\n, \r).

These would throw an error if present in text when evaluating. Now they should be correctly escaped, and still be present in final output. The fix applies to any code that calls the JSEvaluator, including data pipe filter operation (where issue first identified)

• Add tests to JSEvaluator and DataPipe Filter specs

Git Issues

Closes #2184

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

[chrismclarke]

@jfmcquade - could you look into the failing tests? looks to be related to debug repo config changes as mentioned on the call

[jfmcquade]

@jfmcquade - could you look into the failing tests? looks to be related to debug repo config changes as mentioned on the call

That was indeed the issue, fixed as of #49 on the debug repo

[esmeetewinkel]

Thanks both. This indeed resolves the error with the line breaks, functional test passed on the debug deployment.

Filtering the sheet on the facilitator app deployment linked initially is still giving me head aches, it's now spitting out other errors error: SyntaxError: Unexpected identifier 's', error: SyntaxError: Unexpected identifier 'd', error: SyntaxError: Unexpected identifier 're'

List of row IDs, with problematic IDs bold:

what_is_safeguarding during_disclosed_abuse after_disclosed_abuse how_to_report_abuse contact_for_reporting_abuse crisis_hotlines day_1_introduction day_3_check_in day_3_mh_stress_self_talk day_5_goodbye day_4_check_in day_6_final_chat_sessions_5ux faq_h_after_group_ended faq_text_support faq_ground_rules faq_misbehaviour faq_small_group_active faq_h_still_struggling faq_h_sad_to_end faq_no_participation_general

I can't determine what the broken / functioning rows have in common, but I'm guessing this is a similar JS thing? Can open a follow-up issue if it makes more sense to merge this PR first.

[jfmcquade]

Filtering the sheet on the facilitator app deployment linked initially is still giving me head aches, it's now spitting out other errors error: SyntaxError: Unexpected identifier 's', error: SyntaxError: Unexpected identifier 'd', error: SyntaxError: Unexpected identifier 're'

I just had a quick look and wasn't able to replicate either on this feature branch or the master branch. I was setting aritcle_filter to released here and running yarn workflow sync, is that right?

[esmeetewinkel]

I just had a quick look and wasn't able to replicate either on this feature branch or the master branch. I was setting aritcle_filter to released here and running yarn workflow sync, is that right?

Try again now - article_filter was set to filter my testing copy of article_data (article_test_data) which I'd removed again. I've now changed it to refer to article_data.

[jfmcquade]

Thanks. I think the issue is still with newlines, but I'm not quite sure what is causing it. If I log the value of cleanedFuncString from jsEvaluator.ts to the console then it appears that some of the content within a single cell gets correctly logged with visible "\n" characters, but sometimes the newline gets interpreted and actually displays as a newline. I can't work out yet what is causing the discrepancy. Noting here so I can continue looking tomorrow. I've tried tweaking the regexes, but the newlines that do get interpreted seem to be being picked up correctly.

[chrismclarke]

@esmeetewinkel @jfmcquade I think there's 2 potential further issues here.

The first is additional characters that need escaping - I'm not exactly sure the full limits of what can/can't be used (all values essentially get combined to form one very long string of javascript code and executed as a function, and I'm not 100% sure the full limitations of working in this way).

One issue I could identify however is single quotation marks ' breaking because of the way the long string of javascript code uses single quotation marks around any string values detected from a row. So I've pushed 0ee2ce4 which should hopefully fix that and many of the issues seen (tests updated too).

E.g. search for single quotation marks picked up 30 instances which should now be fixed

The second issue is more the question of whether all this data needs to go into the JS Evaluator given that most of it is unused in the evaluation process. E.g. for the filter condition throwing errors it's only the draft column used.

So I've drafted a follow-up pr #2189 (code in 325341c) to provide a potential workaround/optimisation in case any rows do still throw error, although if they do would still be good to identify any specific text characters causing issue to try and add a core fix for

[jfmcquade]

I've just been doing some functional testing. As you mentioned might be the case @chrismclarke (in the description of #2189), the problematic sheet still caused an error on this PR's branch, but not on the branch for #2189. In this case, it was the row faq_h_self_harm which caused the following error to be thrown:

After logging out the value of cleanedFuncString and analysing the output, I found that the section_text_1 key for this row is missing a value:

Upon inspection, it looks like that cell of the sheet contains only newline characters (the highlight colour on my computer is pink, so this shows 2 newline characters selected in the cell):

Deleting the contents of this cell caused the same error to be thrown on a new row, which, after following the same process, turned out to have a cell with a single newline character in also. Having deleted the contents of this cell, the filter runs successfully.

So I'm not sure why it should behave differently, but I think we need to explicitly handle the case of a cell value consisting entirely of newline characters.

[chrismclarke]

So I'm not sure why it should behave differently, but I think we need to explicitly handle the case of a cell value consisting entirely of newline characters.

Nice catch! Yeah definitely a case I hadn't thought of. I'm thinking maybe this should be handled further up the chain (e.g. when syncing sheets), as I can't really think of any time a cell really would want to contain just a new line

Maybe we can turn into a low priority issue for someone to fix in the future? The default parser already has a cleanFieldValues method currently used to remove additional whitespace, so might be the place to add a check/replacement.

[chrismclarke]

Issue added https://github.com/IDEMSInternational/parenting-app-ui/issues/2193 Let me know if you want me to provide a quick fix or leave for future

[esmeetewinkel]

Let me know if you want me to provide a quick fix or leave for future

I think we can leave for future. Thanks!