Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

Missing acute in french in self harm2084 #2182

Closed
wants to merge 7 commits into from
Closed

Missing acute in french in self harm2084 #2182

wants to merge 7 commits into from

Conversation

dengcn
Copy link
Contributor

@dengcn dengcn commented Aug 11, 2020 •
edited
Loading

Fixes #2084

This is the fix to PR2084 Missing acute accent in self harm word section

Description

There are 2 issues in this PR

  1. In the French alarm warning, acute or accented sign were missing
  2. French Flagged field(for alarm) does not match with the title of French alarm warming

Please include a summary of the change.
-In the handling of selfAlarmWordsScan
-In the Language json

Note: In the fix,
The accute accented sign will be preserved in report, also if user miss any accute accented sign in french word. it will also be captured.

For example: Déprimé is defined in the env
If user type in Déprimé ===> déprimé in report
If user type in déprimé ===> déprimé in report
If user type in deprimé ===> déprimé in report
If user type in deprime ===> déprimé in report

Any new packages installed?

No

Give details

Required followup work

Is there anything related to this that still needs to be done (ex: documentation changes).

Checklist:

  • I have updated the azurescript.sh with any new environment variables and added them to the appsettings
  • [ x] I have looked at my code on GitHub and it all looks good (ex: no random commented out code or console.logs)
  • I have added and needed tests for my changes (in particular for new screens)
  • I have added a comment to any confusing code
  • I have added documentation to any modified front-end code. (Or added missing documentation)

@sastels sastels temporarily deployed to rac-prototyp-missingacu-eip1xo August 11, 2020 13:12 Inactive
@lgtm-com
Copy link

lgtm-com bot commented Aug 11, 2020

This pull request introduces 2 alerts when merging 15de80a into 679cdf0 - view on LGTM.com

new alerts:

  • 1 for Missing variable declaration
  • 1 for Unused variable, import, function or class

justinr86
justinr86 reviewed Aug 11, 2020
View reviewed changes
f2/src/locales/fr.json
@@ -14,7 +14,7 @@
"analystReport.reportLanguage": "Langue du rapport",
"analystReport.reportNumber": "Signaler le numéro",
"analystReport.reportVersion": "Version du rapport",
"analystReport.selfHarmString": "mots d'automutilation détectés",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this translation come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming from Harm Word in title already defined and used, and PR raiser wish this the same as title

@sastels sastels temporarily deployed to rac-prototyp-missingacu-eip1xo August 11, 2020 13:54 Inactive
justinr86
justinr86 reviewed Aug 11, 2020
View reviewed changes
f2/src/utils/selfHarmWordsScan.js
@@ -47,24 +45,36 @@ const selfHarmWordsScan = (data) => {
//Scan String for key words. Tokenize and stem to identify root words.
const scanString = (str) => {
try {
let modifiedStr = unidecode(str.toLowerCase())
let modifiedStr = str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why declare a variable that has the same value as the param?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the original copy of data, since modifiedStr will be heavily changed or regrouped in the following code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can consolidate these 2 variables, any others?

@sastels sastels temporarily deployed to rac-prototyp-missingacu-eip1xo August 14, 2020 22:22 Inactive
@justinr86
Copy link
Contributor

From description:

Note: In the fix,
The accute accented sign will be preserved in report, also if user miss any accute accented sign in french word. it will also be captured.

For example: Déprimé is defined in the env
If user type in Déprimé ===> déprimé in report
If user type in déprimé ===> déprimé in report
If user type in deprimé ===> déprimé in report
If user type in deprime ===> déprimé in report

Is this desired outcome? The goal of this code change is to maintain the user's input not correct it.

Also, there seems to be a problem with the report. Though self harm keywords are flagged they are not displayed in the report and there seems to be an encoding issue (not sure if this is related to this change or not), see attached. I submitted a report with the word deprime in the What Happened section.

frenchSubmission

@sastels sastels temporarily deployed to rac-prototyp-missingacu-eip1xo August 17, 2020 20:16 Inactive
@justinr86
Copy link
Contributor

Sorry, I think I misunderstood part of the description. However, there are several failed unit test related to self harm word scan.

@dengcn
Copy link
Contributor Author

dengcn commented Aug 18, 2020

Nice catch on this screen (word missing from warning yellow part) from this single harmful word, there was a bug associated with handling such single word, I will fix that.

@sastels sastels temporarily deployed to rac-prototyp-missingacu-eip1xo August 18, 2020 15:56 Inactive
@dengcn dengcn mentioned this pull request Sep 8, 2020
Closed
@justinr86
Copy link
Contributor

justinr86 commented Oct 16, 2020 •
edited
Loading

What's the status of this PR? Is this code change still required?

@dengcn
Copy link
Contributor Author

dengcn commented Oct 16, 2020

There is an error in the code which needs to be fixed, the pointed PR is still valid, not closed

@justinr86
Copy link
Contributor

There is an error in the code which needs to be fixed, the pointed PR is still valid, not closed

This pull request was created on Aug. 11. Since we are migrating to Azure at the end of the week it might be a good idea to resolve this issue.

@dengcn dengcn closed this Oct 29, 2020
@dengcn
Copy link
Contributor Author

dengcn commented Oct 29, 2020

Close this PR for now, reopen when migration to Azure is ready

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@justinr86 justinr86 justinr86 left review comments

@kevinyang06 kevinyang06 Awaiting requested review from kevinyang06

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing acute accent in self harm word section
4 participants
@dengcn @justinr86 @sastels @DianeLiu2019