[Raisinbread] Fixing help_editor module to display RB instrument instructions #6907

AlexandraLivadas · 2020-08-12T22:19:05Z

Edited
This PR aimed to add data to the help table in Raisinbread so that the data would generate in the Help Editor module. This will allow the Help Editor module to be manually tested.

It was found that there is a bug in this module that causes it not to save/display this information. This issue is resolved in this PR.

Here is a screenshot of the Help Editor module with the new data:

Testing instructions (if applicable)

With a fresh RB install:

Navigate to Help Editor in the frontend and see that data loads for instructions for the BMI instrument.

Link to related issue

Resolves [Raisinbread] No data for help editor module? #5691

christinerogers · 2020-08-14T21:07:48Z

great, can you also comment in one line on whether the help editor seems to work in terms of editing help text?

AlexandraLivadas · 2020-08-14T21:43:23Z

@christinerogers The Help Editor does not correctly save changes made in the frontend to the file contents. I made changes and hit Save. The changes displayed correctly but once I hit Return to Help Editor or navigated back to the main page myself, the changes were actually not saved.

Another issue is that the button Return to Help Editor does not actually return me to the main Help Editor page. Instead, I stayed on the editing content page and the changes I made were not saved.

christinerogers

Hi Alex, see my comment below, the topic column is probably trying to match a valid test name value.
Can you dig further into the code to find out?

Dave's question on the issue here isn't actually addressed until we know that part.

@ridz1208 -- care to review otherwise?

christinerogers · 2020-08-27T17:07:34Z

raisinbread/RB_files/RB_help.sql

@@ -1,5 +1,6 @@
 SET FOREIGN_KEY_CHECKS=0;
 TRUNCATE TABLE `help`;
 LOCK TABLES `help` WRITE;
+INSERT INTO `help` (`helpID`, `parentID`, `hash`, `topic`, `content`, `created`, `updated`) VALUES (123,12,'hash','BMI Instructions','The BMI calculator instrument can calculate the BMI and BMI Classification of a visitor. The two required fields for this instrument are the Date of Administration of the instrument and the Examiner name. The height and weight of the visitor must then be entered in either standard or metric units. The instrument will not accept an entry where the height and weight are given in both units. After this information is given, the instrument will calculate the BMI and BMI Classification of the visitor.',NULL,NULL);


Alex, since this doesn't actually render for the BMI -- per your comment here
I think it might be because of the value in the topic column. If you change this to bmi (which i think would be the relevant test_name value for this instrument) -- what happens?

Could you also take another step here towards resolving it by verifying in the code how the Help display/dropdown actually trying to locate relevant help text? If it's not trying to match the topic column against a valid test_name value -- then what is it trying to match/look for?

Yes I can definitely look into this :)

@christinerogers @driusan So after some digging:
The help file is handled by helpHandler.js. This then calls the modules/help_editor/ajax/help.php file with a GET request.

In THAT file, it first tries to render the help file by either using the module name (which is given by the request parameter testName or using the subtest parameter (in this case "bmi" or "aosi"). If it's not a module, it instead tries to load data from the help table using the request parameter subtest as the identifier. But when I went to an instrument data entry page and used the Developer Tool, I saw that when the GET request is sent to help.php, it only provides the testName parameter in the request and doesn't provide a subtest value, so it never checks if there's a file in the help table and instead just looks for the markdown help file for the Instruments module.

Let me know if this makes sense, I can explain it in more detail if necessary. I don't know how to fix this as of now. I can keep looking to see how to edit the GET request to provide the subtest parameter, but I haven't figured out where to do that yet.

ridz1208 · 2020-09-01T14:30:02Z

@zaliqarosli can you team up with @AlexandraLivadas to figure out what is happening in this module ?

our options would be to either fix it if it doesnt work as intended or remove the module entirely

zaliqarosli · 2020-09-01T15:49:00Z

@ridz1208 yeah sure @AlexandraLivadas wanna slack me or get on a call to explain what's going on?

ridz1208 · 2020-09-09T19:06:35Z

@zaliqarosli @AlexandraLivadas have you guys firgured it out ? is it working for bmi specifically now ?

ridz1208 · 2020-09-09T19:14:51Z

fixes #5691

zaliqarosli · 2020-09-09T19:48:58Z

@ridz1208 yeah i fixed it - sent a PR to this PR branch. @AlexandraLivadas can you merge the changes?

driusan · 2020-09-10T13:03:37Z

modules/help_editor/js/help_editor_helper.js

@@ -1,5 +1,3 @@
-import swal from 'sweetalert2';


why is this downgrading from swal2 to swal1?

When @zaliqarosli was making the changes, she said that this statement threw an error. However, when I added this line back on my local version, I did not get the same error. I'm not sure why this was causing issues for her but not me.

I don't think it should be downgraded.

undoing the changes on this file is still giving me the error:

@laemtl also reported seeing this error in #6940. (Laetitia, any chance you could give this PR a test, with and without these changes to this file?)

this PR as is fixes all issues. @driusan @HenriRabalais @maltheism any of you know how to properly fix this without downgrading to swal1?

Yes sure! I'm also working on #7018 so it might be helpful to fix this problem with swal.

I'm not sure I understand why the file can't just be moved from the js directory to jsx and added to the webpack config so that webpack compiles it and it can use swal2?

@driusan Eslint fails with /var/www/html/loris/modules/help_editor/jsx/help_editor_helper.js
33:13 error A function with a name starting with an uppercase letter should only be used as a constructor new-cap

markdownContent = document.createElement('div'); ReactDOM.render( RMarkdown({content: content}), markdownContent );

I pushed a commit which fixes the pb with the fix found here which changes the eslint rules.

wouldn't it be kind of against our repo standard that we're calling a non-react non-jsx file a jsx file and putting it under that directory? help_editor_helper.js isn't a component and doesn't need to be compiled. there's a way to call sweetalert2 with normal JS:

but using require() also throws an error. surely just making the file a jsx file isn't the right move?

@zaliqarosli nodejs, browserify, or webpack supports require() but not javascript on the browser. The browser shouldn't have access to node_modules on the server anyways.

I think it's not great that we cannot use import in the js directory as well. It would be nice to keep helper code away from the component directory. Although, I do like the thought of js directory just having compiled js code from jsx with nothing else.

@zaliqarosli You are right, I migrated my fix to #7018. I'm not sure the way I tackled it is the best. I kind of agree that it is weird to have this file under jsx/. I wonder if reactifying it should be preferable? Isn't the plan to completely remove the jQuery dependencies in the future?

php/libraries/NDB_BVL_Instrument.class.inc

zaliqarosli · 2021-07-05T17:58:57Z

is discussion still required here?

AlexandraLivadas · 2021-07-05T18:02:39Z

@zaliqarosli No, I don't think so. I think this was approved a while ago and just needed to be tested and reviewed again on the 23.0 branch

zaliqarosli

everything else looks good. just wondering about one leftover comment. i am happy to approve this PR as is though

modules/help_editor/ajax/help.php

zaliqarosli · 2021-07-05T19:31:32Z

could someone test this? i'm having trouble with my dev VM. @jstirling91 would you be interested?

zaliqarosli

everything else looks good! will test now

modules/help_editor/ajax/help.php

Co-authored-by: Zaliqa <zaliqa.rosli@mcin.ca>

zaliqarosli · 2021-07-06T15:47:29Z

@AlexandraLivadas any chance you could edit the help_editor .md help file to say "This module displays existing help content for LORIS instrument pages" instead of what's there?

…AlexandraLivadas/Loris into 2020-08-12-addDataForHelpEditor

ridz1208 · 2021-07-06T18:00:59Z

@driusan this one is in your hands, it's going to 23 since it's a bugfix requested by IBIS

modules/help_editor/help/help_editor.md

zaliqarosli

LGTM!!

modules/help_editor/ajax/help.php

ridz1208 · 2021-07-08T17:59:06Z

@driusan i think this one is yours as well

zaliqarosli

ill approve again since the last commit. also retested

AlexandraLivadas added the RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset label Aug 12, 2020

AlexandraLivadas requested review from ridz1208 and christinerogers August 12, 2020 22:19

AlexandraLivadas mentioned this pull request Aug 12, 2020

[Help Doc] Added help file for instrument data entry #6902

Merged

AlexandraLivadas force-pushed the 2020-08-12-addDataForHelpEditor branch from 205bd9f to ee3f055 Compare August 13, 2020 16:12

AlexandraLivadas mentioned this pull request Aug 17, 2020

[Help Doc] No help document for the instruments module #6906

Closed

christinerogers mentioned this pull request Aug 27, 2020

[HelpEditor] Not saving changes #6940

Closed

christinerogers requested changes Aug 27, 2020

View reviewed changes

ridz1208 assigned zaliqarosli and AlexandraLivadas Sep 1, 2020

driusan reviewed Sep 10, 2020

View reviewed changes

php/libraries/NDB_BVL_Instrument.class.inc Show resolved Hide resolved

This was linked to issues Sep 21, 2020

[HelpEditor] Not saving changes #6940

Closed

[Help editor] Edit Help content > Preview does not work #6968

Closed

spell00 mentioned this pull request Sep 21, 2020

[API] Adding an image in the docker container for Images integration tests #7052

Closed

laemtl force-pushed the 2020-08-12-addDataForHelpEditor branch 4 times, most recently from 7ce172c to bca66c8 Compare September 22, 2020 02:31

christinerogers previously approved these changes Sep 22, 2020

View reviewed changes

zaliqarosli removed Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix Passed Manual Tests PR has undergone proper testing by at least one peer labels Jul 5, 2021

zaliqarosli removed the Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties label Jul 5, 2021

zaliqarosli reviewed Jul 5, 2021

View reviewed changes

modules/help_editor/ajax/help.php Show resolved Hide resolved

zaliqarosli approved these changes Jul 5, 2021

View reviewed changes

AlexandraLivadas added 3 commits July 6, 2021 10:42

Reorganizing try-catch statement

4859ceb

Rearranging logic

a748b7b

Checkstatic errors

e1f47cb

zaliqarosli requested changes Jul 6, 2021

View reviewed changes

modules/help_editor/ajax/help.php Outdated Show resolved Hide resolved

Remove whitespace

d1ce28e

Co-authored-by: Zaliqa <zaliqa.rosli@mcin.ca>

AlexandraLivadas added 2 commits July 6, 2021 13:30

Adding instructions to md file

05f44dd

Merge branch '2020-08-12-addDataForHelpEditor' of https://github.com/…

722aa7e

…AlexandraLivadas/Loris into 2020-08-12-addDataForHelpEditor

ridz1208 approved these changes Jul 6, 2021

View reviewed changes

zaliqarosli requested changes Jul 6, 2021

View reviewed changes

modules/help_editor/help/help_editor.md Outdated Show resolved Hide resolved

Update help_editor.md

0a3a209

zaliqarosli approved these changes Jul 6, 2021

View reviewed changes

zaliqarosli added the Passed Manual Tests PR has undergone proper testing by at least one peer label Jul 6, 2021

zaliqarosli reviewed Jul 6, 2021

View reviewed changes

modules/help_editor/ajax/help.php Outdated Show resolved Hide resolved

Update modules/help_editor/ajax/help.php

6bb478c

zaliqarosli approved these changes Jul 8, 2021

View reviewed changes

driusan merged commit 92d8b80 into aces:23.0-release Jul 12, 2021

driusan added this to the 23.0.5 milestone Aug 16, 2021

AlexandraLivadas mentioned this pull request Dec 2, 2022

[Raisinbread] No data for help editor module? #5691

Closed

zaliqarosli mentioned this pull request Dec 20, 2022

Loris instrument help not working #7499

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Raisinbread] Fixing help_editor module to display RB instrument instructions #6907

[Raisinbread] Fixing help_editor module to display RB instrument instructions #6907

AlexandraLivadas commented Aug 12, 2020 •

edited

Loading

christinerogers commented Aug 14, 2020

AlexandraLivadas commented Aug 14, 2020 •

edited

Loading

christinerogers left a comment

christinerogers Aug 27, 2020 •

edited

Loading

AlexandraLivadas Aug 27, 2020

AlexandraLivadas Aug 27, 2020 •

edited

Loading

ridz1208 commented Sep 1, 2020

zaliqarosli commented Sep 1, 2020

ridz1208 commented Sep 9, 2020

ridz1208 commented Sep 9, 2020

zaliqarosli commented Sep 9, 2020

driusan Sep 10, 2020

AlexandraLivadas Sep 14, 2020

driusan Sep 15, 2020

zaliqarosli Sep 21, 2020 •

edited

Loading

laemtl Sep 21, 2020 •

edited

Loading

driusan Sep 21, 2020

laemtl Sep 21, 2020 •

edited

Loading

zaliqarosli Sep 21, 2020

maltheism Sep 21, 2020 •

edited

Loading

laemtl Sep 22, 2020 •

edited

Loading

zaliqarosli commented Jul 5, 2021

AlexandraLivadas commented Jul 5, 2021

zaliqarosli left a comment •

edited

Loading

zaliqarosli commented Jul 5, 2021

zaliqarosli left a comment

zaliqarosli commented Jul 6, 2021

ridz1208 commented Jul 6, 2021

zaliqarosli left a comment

ridz1208 commented Jul 8, 2021

zaliqarosli left a comment •

edited

Loading

[Raisinbread] Fixing help_editor module to display RB instrument instructions #6907

[Raisinbread] Fixing help_editor module to display RB instrument instructions #6907

Conversation

AlexandraLivadas commented Aug 12, 2020 • edited Loading

Testing instructions (if applicable)

Link to related issue

christinerogers commented Aug 14, 2020

AlexandraLivadas commented Aug 14, 2020 • edited Loading

christinerogers left a comment

Choose a reason for hiding this comment

christinerogers Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

AlexandraLivadas Aug 27, 2020

Choose a reason for hiding this comment

AlexandraLivadas Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

ridz1208 commented Sep 1, 2020

zaliqarosli commented Sep 1, 2020

ridz1208 commented Sep 9, 2020

ridz1208 commented Sep 9, 2020

zaliqarosli commented Sep 9, 2020

driusan Sep 10, 2020

Choose a reason for hiding this comment

AlexandraLivadas Sep 14, 2020

Choose a reason for hiding this comment

driusan Sep 15, 2020

Choose a reason for hiding this comment

zaliqarosli Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

laemtl Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

driusan Sep 21, 2020

Choose a reason for hiding this comment

laemtl Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

zaliqarosli Sep 21, 2020

Choose a reason for hiding this comment

maltheism Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

laemtl Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

zaliqarosli commented Jul 5, 2021

AlexandraLivadas commented Jul 5, 2021

zaliqarosli left a comment • edited Loading

Choose a reason for hiding this comment

zaliqarosli commented Jul 5, 2021

zaliqarosli left a comment

Choose a reason for hiding this comment

zaliqarosli commented Jul 6, 2021

ridz1208 commented Jul 6, 2021

zaliqarosli left a comment

Choose a reason for hiding this comment

ridz1208 commented Jul 8, 2021

zaliqarosli left a comment • edited Loading

Choose a reason for hiding this comment

AlexandraLivadas commented Aug 12, 2020 •

edited

Loading

AlexandraLivadas commented Aug 14, 2020 •

edited

Loading

christinerogers Aug 27, 2020 •

edited

Loading

AlexandraLivadas Aug 27, 2020 •

edited

Loading

zaliqarosli Sep 21, 2020 •

edited

Loading

laemtl Sep 21, 2020 •

edited

Loading

laemtl Sep 21, 2020 •

edited

Loading

maltheism Sep 21, 2020 •

edited

Loading

laemtl Sep 22, 2020 •

edited

Loading

zaliqarosli left a comment •

edited

Loading

zaliqarosli left a comment •

edited

Loading