search

OpenFn / grassroot-soccer

Grassroot Soccer CommCare-Salesforce integrations
https://openfn.github.io/grassroot-soccer/
0 stars 0 forks source link

Add cleaning rule at the beginning of all jobs to replace emojis with an empty string #30

Closed daissatou2 closed 2 years ago

daissatou2 commented 2 years ago

Describe the bug

Some of the jobs are failing because of an emoji in the message. Write a cleaning rule at the beginning of all jobs to replace emojis with an empty string.

To Reproduce

  1. Here is a link to a failed run on OpenFn.org which is indicative of the bug: https://www.openfn.org/projects/pd3yk4/runs/r7x6984w

expression.js

Link to the job itself in Github: All jobs here https://www.openfn.org/projects/pd3yk4/jobs

state.json

https://www.openfn.org/projects/pd3yk4/messages/mv7e5nd6

Expected behavior

The job should remove the emoji.

To test/resolve

  1. After the desired output is working locally (from the CLI), please [push commits to master || open a pull request].
  2. [Please test the change on OpenFn.org by re-running this run (https://www.openfn.org/projects/pd3yk4/runs/r7x6984w) and confirming success.]
lakhassane commented 2 years ago

@daissatou2 do we know exactly all the fields we should clean? Doing it for all fields for all jobs might be time consuming or should I focus only on CommCare_Ext_ID__c

lakhassane commented 2 years ago

Also there are 24 jobs. Should we do it for all 24 or do you have a potential list of jobs using CommCare_Ext_ID__c that are failing and that we should focus on

daissatou2 commented 2 years ago

@lakhassane let's just focus on CommCare_Ext_ID__c for these jobs:

lakhassane commented 2 years ago

@daissatou2 I remove emojis for some (see checked jobs in your comment).

For some of the remaning I don't see the emojis or I don't have a message for that job that can help me see the issue, so I'm holding them off until we have a failing run for them so I can see better.

aleksa-krolls commented 2 years ago

@daissatou2 have you tested this and reprocessed affected runs? If yes, can we close?

daissatou2 commented 2 years ago

@lakhassane here is an example of a message that still has the Dove: https://www.openfn.org/projects/pd3yk4/runs/0615275f-70cb-7e05-8c8f-064602b086e7

It is from jobs/2.d.upsertRegisterParticipant.js which I noticed hasn't been checked.

lakhassane commented 2 years ago

@taylordowns2000 @elias-ba I updated the list of jobs that potentially make use of a message that has intervention_name value for mapping (see the comment with the checklist). intervention_name is the field with the dove.

aleksa-krolls commented 2 years ago

@elias-ba Is this good to test on prod? Do any changes need to be made to the adaptor versions or anything?

@daissatou2 pls test today and let Elias know if any feedback.

elias-ba commented 2 years ago

The initial issue is okay to test on prod. But the run itself has a missing fiel error. @daissatou2 will help with that today.

elias-ba commented 2 years ago

@daissatou2 I am letting you do your tests on prod and ping if needed. Thanks

daissatou2 commented 2 years ago

@elias-ba I am seeing the same error when I rerun on prod: https://www.openfn.org/projects/pd3yk4/runs/06157022-75d7-79b6-a0c9-f5f08d1dc9f0

elias-ba commented 2 years ago

Hey @daissatou2 can you make sur you use language-salesforce@v2.7.0 version in the job ? You see the little cloud icon, when you click on it you can specify the name and version of the adaptor to use. And try with that.

@taylordowns2000, @lakhassane how can we set the default adaptor to v2.7.0 ?

daissatou2 commented 2 years ago

@elias-ba I updated the adaptor and same error: https://www.openfn.org/projects/pd3yk4/runs/06157048-4eb5-749e-bc62-2d348e321ef7

elias-ba commented 2 years ago

Okay normal I see the problem. Currently I only added the new version of the removeEmoji function in this job: https://www.openfn.org/projects/pd3yk4/jobs/jyp9j4. And that's not the one you're using in this run: https://www.openfn.org/projects/pd3yk4/runs/06157048-4eb5-749e-bc62-2d348e321ef7.

I can add the function to the other jobs after my break and let you test.

But can you test with the job above ? Just to make sure the function is working great on prod before we use it in the other jobs.

daissatou2 commented 2 years ago

Oh I don't have a run that I can test with for Upsert Pre Challenges. Unfortunately, the old run links do not work. Please let me know when Upsert Register Participant is ready to test :)

elias-ba commented 2 years ago

Okay @daissatou2 I will ping. But where can I find the list of fields to clean in all the jobs ? Shouldn't this information be in the mapping ? Is there any doc that can help me ?

daissatou2 commented 2 years ago

@elias-ba It's in the issue on this comment. I'll update the original comment but we are focusing on CommCare_Ext_ID__c for the jobs listed.

elias-ba commented 2 years ago

Thanks a lot @daissatou2.

elias-ba commented 2 years ago

@daissatou2 can you check the new version of the Upsert Register Participant in this run: https://www.openfn.org/projects/pd3yk4/runs/0615738f-b908-7547-bcbe-829e4b8f64d0 ? I tested in prod and it's successful now. Tell me if that's what you're expecting. Meanwhile, I continue to propagate the removeEmoji function to other jobs.

daissatou2 commented 2 years ago

YAY! Thank you!

elias-ba commented 2 years ago

Coool does that mean that we can check Upsert Register Participant in this checklist ?

elias-ba commented 2 years ago

Upsert Post Challenges seems to be working too, is that right ?

elias-ba commented 2 years ago

I have checked all the jobs in the checklist as they've all now the latest version of the language-salesforce adaptor and the removeEmoji function and their runs are all successful. Please @daissatou2 give it another round of test asap and tell me if there's anything that needs to be fixed. Otherwise I think this issue can be closed. Thanks

taylordowns2000 commented 2 years ago

@daissatou2 writes: "Hey @elias-ba we are seeing the same emoji error now: https://www.openfn.org/projects/pd3yk4/runs/06168202-196a-70dc-ac2a-7891ce7d7556"

@aleksa-krolls writes: "@elias-ba We will pick this up again on Monday, but can you please leave a status update here on GH regarding where you left off yesterday? (Want to make sure there's not a quick here by switching the adaptor versions from v2.7.1 to something else cc @taylordowns2000)"

Update

@elias-ba didn't continue working on this (at my instruction) after 420pm UTC yesterday as we were both unable to reproduce the error due to the credential issue. Now that we've got working creds and have reproduced the error, I think I see how this could be addressed and will raise a PR on language-common, but want to discuss this with Aicha and Elias on Monday because I'm not certain I follow why/how the code could have been working at all 14 days ago when this issue was marked as closed. After we discuss on Monday, a fix should be fairly quick.

aleksa-krolls commented 2 years ago

@daissatou2 are we all good here? Let's close this out, if yes. We should have all jobs on adaptor v2.7.2 https://openfn.slack.com/archives/C017ELVRSM8/p1634555065003800?thread_ts=1634200704.001300&cid=C017ELVRSM8