Hi Professor,

Here I am attaching the code I have written so far for Context Sensitive help system for Yioop. I would like to request you to do a code review as I have been developing for some time now and the code has been growing. This also includes enhancements to Yioop system I have worked on. Below is a brief summary of my changes:

- Web service to retrieve wiki content with page name passed as a query param

ex: ?c=api&group_id=7&arg=read&a=wiki&YIOOP_TOKEN=HSK85Q-dy0E|1412836881&page_name=browse_url

- Java script code in basic.js to fire HTTP GET to grab wiki contents and toggle help frame to display parsed help content.

- CSS styling for Help frame display for Left, right aligned languages as well as mobile.

- On Edit wiki page, When the user clicks on Save, the page will go into read mode directly.

- Added a Preview button to parse the wiki content on client side and display in wiki preview Div.

- Add a direct link to Wiki beside each group link on manageGroups page.

Thanks Eswara

Hey Eswara,

If you want me to look at your patch you need to remember to add either cpollett or administer to the user list. Otherwise, I won't get an email letting me know about it.

Chris

I've looked at your patch. Two things I noticed. The first is minor: there is one place you have a multi-line comment using // rather than /* */. The second comment concerns the code: function getEditLink(target_c, csrf_token, group_id, page_name) +{ + return '?c=' + target_c + + '&YIOOP_TOKEN=' + csrf_token + + '&group_id=' + group_id + + '&arg=edit' + + '&a=wiki' + + '&page_name=' + page_name; +} YIOOP_TOKEN is not fixed across all yioop instances. It is the default value of the the CSRF_TOKEN define, but people can change it. So it will be different for different machines so the above code will break in that case. Please fix this and make a new patch.

Best, Chris Best, Chris

Hi Professor,

Thanks for your feedback. I will make a note of adding admin/cpollett as user while submitting a patch.

Please find the new patch atatched with the changes you suggested.

- Fixed multiline comments. - Made the CSRF token "name" dynamic.

Thanks Eswara

Hey Eswara,

Your patch needs to have a valid email address to be signed off. Also, when you add things to $available_controllers it should be in alphabetical order.

Best, Chris

Hi Professor

Please find the new patch created with my email address. Also this includes the different views for "Recent Group Activity"

Thanks Eswara

Patch v3 doesn't apply. I get the message: error: patch failed: .gitignore:1 error: .gitignore: patch does not apply Patch failed at 0001 before addign icons The copy of the patch that failed is found in:

  /Applications/MAMP/htdocs/git/yioop/.git/rebase-apply/patch

Hi Professor,

I am not sure why the commit with binary files is causing a problem. I created a new unified patch and tested it by applying to master as well. Please find cs298_v4.patch.

please let me know If you are still seeing issues applying patch.

Thanks, Rajesh

This time I was able to apply the patch. Here's some feedback on changes I'd like before pushing it.

(1) The single feed and category icons look different in size. This might be an optical illusion caused by differences in padding in the two icons, but is still disconcerting. The category icon should not be bar, two indented bars, bar, indented bar. Instead, just use bar indented bar, bar indented bar. (2) The single feed and category buttons only work when using the admin (non-collapsed) controller. They don't show up at all when using the group (collapsed) controller. (3) Category view is not doing sufficient isset checks. It generates Notices like: NOTICE: Undefined index: SUBTITLE at line 104 in /Applications/MAMP/htdocs/git/yioop/views/elements/groupfeed_element.php

 in GroupfeedElement->render, line 81 in /Applications/MAMP/htdocs/git/yioop/views/admin_view.php 
 called from AdminView->renderView, line 205 in /Applications/MAMP/htdocs/git/yioop/views/layouts/web_layout.php 
 called from WebLayout->render, line 136 in /Applications/MAMP/htdocs/git/yioop/views/view.php 
 called from View->render, line 258 in /Applications/MAMP/htdocs/git/yioop/controllers/controller.php 
 called from Controller->displayView, line 214 in /Applications/MAMP/htdocs/git/yioop/controllers/admin_controller.php 

...

NOTICE: Undefined index: SUBTITLE at line 122 in /Applications/MAMP/htdocs/git/yioop/views/elements/groupfeed_element.php

 in GroupfeedElement->render, line 81 in /Applications/MAMP/htdocs/git/yioop/views/admin_view.php 
 called from AdminView->renderView, line 205 in /Applications/MAMP/htdocs/git/yioop/views/layouts/web_layout.php 
 called from WebLayout->render, line 136 in /Applications/MAMP/htdocs/git/yioop/views/view.php 
 called from View->render, line 258 in /Applications/MAMP/htdocs/git/yioop/controllers/controller.php 
 called from Controller->displayView, line 214 in /Applications/MAMP/htdocs/git/yioop/controllers/admin_controller.php 

... (4) In the wiki, I don't see what the Preview button does which is useful. When I click either Save or Preview it says that the page is saved. So it does not seem to be previewing changes. For the extra-button I would really like a strong use-case for why it is needed. (5) You are also adding a lot of code for the help wiki, but no way the end user can test it. This needs to be testable before the patch goes live. Have at least one place where help has been added so I can test this new functionality.

Please address the above and make a new patch. Please also try to reduce the trailing whitespace errors in your patches. As my comments are getting more substantive you are getting closer to a patch that can go live.

Best, Chris

Hi Professor,

I have made all required changes as you suggested, please see my comments below:

(1) Icon fixes - Fixed icons as per your suggestion. (2) The single feed and category buttons only work when using the admin - This is fixed, also When moving from expanded controller to collapsed controller, view is also retained. (3) PHP Notices - These errors and another on the same page are all fixed by checking using isset()

(4) Preview button - When you click on Preview, that doesn't save the wiki content. I am not sure why this happened when you tried. I tried again multiple times and clicking on Preview just shows the preview. The idea behind having a separate preview button is to save additional POST call to the server, Again, this is something I found very handy on Media wiki.

(5) Have at least one place where help has been added so I can test this new functionality. - I added a sample help button on manageGroups action - ?c=admin&YIOOP_TOKEN=obbrFXEgrts|1413522871&a=manageGroups. Please let me know If you need the buttons rendered anywhere else.

Please see attached cs298_v5.patch attached. Please let me know If you see any issues.

[Edit] - Also I am not sure how to prevent white spaces in my patches. I will do some lookup in git docs to understand more. Please let me know If you face any issues applying the patch.

Thanks Eswara

Hey Eswara,

I applied your patch and push it. Can you briefly say how to connec the help button with a wiki page. That button you added didn't seem to be associated with anything.

Chris

Hi Professor,

Currently the button code assumes the wiki group to be 7. So you can create a new group and add the groupId in the basic.js.

- you can flip the var help_group_id in the function "displayHelpForId" to whatever the help group id is. - Now You can create new wiki articles and use the name of the wiki article in the renderHelpButton helper method. example below:

e($this->renderHelpButton("title here", $data[CSRF_TOKEN],

                           $_REQUEST['c']));

Btw, This will not be required, once I complete the seamless upgrade to Context sensitive help with on the fly creation of Wiki group.

Thanks Eswara

Hmmm. I hope a second patch for that comes soon. (1) basic.js is not allowed to have hard-coded constants or make use of PHP. (2) You are using 8 spaces rather than 4 spaces for tabbing on code you introduced in basic.js. (3) Although I said to put renderHelpButton in View, you should instead make a Helper subclass. That way your code isn't loaded every time any view is made (even on those pages that don't have wiki-help). (4) I still can't get any wiki help pages to work. I just get data equal null errors in the Javascript console when I set the group id to be of an existing group. (5) The word Edit is hard-coded and not localizable in displayHelpForId. (6) The point of the square brackets around Edit is accessibility. But you put them as part of the link text rather than outside the link text which defeats their purpose. (7) Don't have global uses of anonymous function definitions like you did for the function get in basic.js. Make it match the other function definitions in basic.js (8) The function name get is too vague and non-specific. There is already a getPage function in basic.js. Your code seems to be adding some functionality related to it, so maybe call it getPageWithCallback. (9) Output html should be XHTML 5. So you can't write things like <hr>. Instead it should be <hr /> (10) Each parameter or return value in a comment needs a short description next to it. (11) target_c --> target_controller (no abbreviations in variable names) (12) {type} as the type is too vague. (13) Don't use braces around types. Write String not {String}. (14) Javascript has a Function object type, it does not have a function type. (15) @returns --> @return

Hi Professor,

I am working on the changes as we speak. I will make the changes asap , will test them and will send a second patch.

for (2) I am not sure which lines they are , can you please let me know.

for(4) : Are you also using the same name as the wiki page name to render the help button? there should be an attribute page-name="Wiki Title Here" rendered in the button element.

for (12) (13) (14) (15) they were auto generated by netbeans, I will make the changes handwritten.

Thanks Eswara

Hi Professor,

I worked on all the changes including the replacement of tab characters with 4 spaces. However I am unable to send the localized content to the layout using the $data variable, I am setting the $data['SCRIPT'] in a new helper subclass that I created. I tried passing the $data by reference from the class which calls the helper, however the $data doesnt seem to be passed by reference at all places. Is there a different way I can set the SCRIPT content without using $data?

If you are ok with using the patch without the localization change for "Edit" , please see the patch attached.

Thanks eswara

Why don't you add a parameter to displayHelpForId for the strings to localize?

The Preview tab still is saving rather than previewing when I test it. I just tested it again because I was wondering how you handled resources on the page like images. I would be inclined to remove the preview button.

Hi Professor,

- Thanks for the advice, I added the tl object as a data-tl attribute.I thought it would be nice to have less params, and also be flexible to add more localization entires in the future. - I have removed the Preview button now. Also fixed all the changes that came with it. Btw, the reason why the default submit action is happenign is bacause "event.preventDefault()" might not be working in your browser, I think you are using mozilla firefox and firefox required the event to be passed as a param other wise its undefined.

Please find the new patch attached.- cs298_v6.1

Thanks Eswara

Hey Eswara,

Your patch didn't apply. Did you do a git pull before making it?

Chris

Hi professor,

I think I dint pull the last few commits. Here is a new patch with all the latest commits pulled, please try this.

cs298_v7.patch

Eswara

Hey Eswara,

That patch undoes all my commits from today. Please make a patch that only affects the necessary files.

Chris

Hi professor,

Finally I think I have a working (no confusion involved :)) patch. Please try cs298_v8.patch.

[Edit] : addied proper git commit message in the patch.

Thanks Rajesh

Hey Rajesh,

I haven't committed or changed anything on my part between the times of your patch 7 and 8 but 8 does not apply. It pretty much complains on each file listed. At first I thought it was only complaining on the configure.ini in which case I could remove them, but it complains on basic.js, etc.

Chris

Hi Professor,

I did check the patch by aplying it on master before sending yesterday, and I could see only one whitespace warning but it did apply. May be more commits on top of what I had was the problem.

I did pull again just a few minutes ago and the latest commit I have is

- More careful checking of crawl time in fetch_controller.php, a=chris

Now I created a patch on top of it , and tried applying it and it went through.

➜ $ yioop git:(master) git am --signoff < ../cs298_v9.patch Applying: Fixed all Issued suggested by Professor, /Applications/XAMPP/htdocs/yioop/.git/rebase-apply/patch:877: trailing whitespace.

                 value="" maxlength="<?php e(SHORT_TITLE_LEN); ?>"

warning: 1 line adds whitespace errors.

Just one warning.

Please try cs298_v9.patch and let me know how it works.

[Edit] Also I have been working on the back button changes over the weekend. I have tested the changes and they look pretty stable. Once this patch is applied, I will create another patch on top of it.

- Eswara Rajesh

HI Professor,

I am sorry about the flower brackets placement for JS functions. I should have checked that. The Js-beautifier tool I have used has reset all the flower bracker placements and added extra white spaces. I will avoid that.

Thanks Eswara

I'll get you to demo setting up wiki help on Tuesday. Otherwise, the patch worked and look good. I have since gone through and edited the formatting issues I still saw. For example, for future reference, just doing a search on SHORT_TITLE_LEN locates the extra whitespace character at the end of the line indicated by git. Other changes I made: pull brackets outside of link url for wiki link in Manage Groups. Made Javascript docs all non-Javadoc, and vertically aligned braces on function definitions.

Your changes are pushed live now. I will keep this issue open till the "seamless upgrade to Context sensitive help with on the fly creation of Wiki group" patch appears that you mention above. You can make a new issue for the back button changes. Thanks again for your hard work!

Thanks professor, I will merge the Back button changes and create a new patch before end of day today. The back button changes are pertaining to Wiki edit page only.

Summary:

When the user navigates to the Wiki edit page from a "help-open" page, they will see the back button. Clicking on back button will navigate the user to the "help-open" page, with the help article open.

Also the back button stays in place even if the user navigates from edit -> read page or edit > edit expanded page or edit -> edit expanded > read expanded etc cases that are possible for the user to navigate.

As this patch is pertaining to wiki edit page, Should I still create a new issue for that patch or can I share the patch in this issue?

Eswara,

Hi Eswara,

Make a new issue. The back button issue will still need to be fixed in other places (probably by me). So it is a good idea to have a place to track that progress.

Best, Chris

Ok sure, I will do it in a new issue.

Thanks! Eswara

Hi Professor,

Thanks for pushing the patch(for Back button changes). I have a working prototype for the seamless integration of Help into New/Existing installations too. I will complete testing, work on other changes and fixes If any, and will incorporate them in the next patch for this issue.

Thanks Eswara.

Hi professor,

Please find the patch attached for Seamless deb integration and other fixes. I have included a summary of all changes below:

cs298_v10.patch

[Edit] adding a minor fix for PHP 5.3 [Edit2] Fixed whitespace errors. Replacing the same patch with removed Whitespace errors.

- Changes for seamless DB upgrade/new creations. - Upgrade db works on up-reving the version to 25 (old) and for new createDb will take care of Group data insertions.

• Bug fixes and refactoring.

- Fixes issue with h2 styles inside wiki html defaulting to blue styling.

- Fixes an issue with PHP <= 5.3 incompatible http_response_code, using a abckward compatible way from now on.

- Behavior of the way wiki toggle help is changed. clicking on help will only open the help. this way, clicking on a help button will refresh the help when another help article is already open.Close is now done with a close button.

- Fixes an issue with help not auto-opening when spaces are used in help name. querySelectorAll chokes with an invalid CSS selector.

-Fixes issue where back-button is not populated when the user : uploads resource, deletes resource, goes to history page. Also the default back button on wiki history page loads when only the wiki back button is not populated.

- Remove last param from help button render, instead call $_REQUEST[‘c’] directly in render method.

- Fixes Issue where back button does not redirect back properly when child help-edit is initiated from child pages.

There are a couple of git whitespace errors, my code doesn't have nay whitespace but I am not sure why git is giving a warning. Please let me know your feedback.

Thanks Eswara

Hi Professor,

Please find the patch attached.

cs298_v11.patch

[Edit] Replacing patch with minor fixes. Also added a minor fix for webkit based browsers. Issue found while testing on phantomjs.

This includes all the fixes we discussed this morning, except the action attribute removal for all post forms. I will work on it and will create a new issue for that.

summary of latest commit:

- Bugfixes. - Fix http_response_code for api send 200 OK HTTP response code.

- make wiki GROUP pub req join 

- Fix wiki add-to-page to fix issue with usage of {{ instead of (( - Remove back_arrows for text "Back" - Add Localization entry for wiki question mark. - Limit "back to help article" functionality only to read/edit args.

Thanks Eswara

Hey Eswara,

Thanks for the latest patch. It has been applied.

Best, Chris

Hi Professor,

Thanks for apying the patch. I have a question regarding parsing wiki markup for resources. How does the PHP wiki parser identify whether a resource is an image or not? Is it based on the file extension? or mime type? I did look at the wiki_parser.php but was not able to find any code specific to this.

I am looking to replicate the same behavior for the JS wiki parser. I am currently defaulting all resources to an image, however I need to identify whether a resource is an image or not. Not sure how to identify the images, as the markup doesn't have any reference to type of the resource.

Thanks in advance. Eswara

Hi Professor,

Here is a new patch with some fixes and features you suggested. Below is a summary :

- Remove Unneeded action attributes with empty values on all Forms - Added Keyboard shortcuts for Bold,Italic & Underline. - Fixes for hyperlink wiki parsing. - Added definition lists for wiki. - Added wiki parser for definition lists.

Please find cs298_v12.patch.

Thanks Eswara

Hi Professor,

I have made the changes suggested by you this morning, Please find the patch cs298_v12.patch attached.

summary : - Remove all instances of action='#' action="#" action='./' action="./" - Group Definition list along with other list buttons. - Fix for definition list hover icon. - Resource parsing for images,files and videos.

Thanks Eswara

Hey Eswara,

I am applying the patch. I noticed that if a wiki page is switched to be a presentation, the presentation icon in the editor is have way through the different kinds of lists. Can you put it to one side or the other?

Best, Chris

Hi Professor,

Please see attached cs298_v13.patch. Just one minor change to move the slide button before lists.

Thanks Eswara

Hi Eswara,

Can you tell me where you actually need setupWikiHelpBackLogic? I don't like this code being executing on every controller. Also, you are not cleaning request variables before using them which is a security hole. You have also added a lot of code to basic.js which is only related to wiki stuff rather than core functions. I am wondering if this should put over in wiki.js or elsewhere? Get rid of getWikiHelpPages() and default_help.db in createdb.php . Just directly have the pages in createdb.php so they are easily editable.

Chris

Hi Professor,

Thanks for the feedback. The wikiHelpBackLogic is required on admin & group controllers and only when arg = read or edit. Currently the back params are setup only when the arg is read/ edit. Please let me know what changes more can i make.

• I will clean the request params.

• Also i will get rid of getWikiHelpPages method and will populate pages like you suggested.

• For the wiki help javascript code, should I move it to another JS file called "help.js"? I thought the "wiki.js" is for the wiki editor only , keeping the wiki editor logic separate.

Please let me know your suggestions so that i can create another patch and send it asap.

Eswara

Hi Professor,

I tried to implement all the issues you pointed. Please see a summary below:

- Execute Wiki help back logic only-> c=admin or group &&a=wiki && arg= edit or read - Move Help Javascript source to a new file. Load the help.js only when atleast one button is rendered on the page. - Fixes help issues that were caused by the latets changes to Pagetitle. - Clean $_REQUEST params before using. - Populate help articles in create db by directly, remove logic that fetches pages from sqlite db.

Please find the patch cs298_v14.patch. Please let me know if you have any concerns.

Eswara.

Hey Eswara,

Thanks for the patch. I had been working on something, which I have partially cut short and pushed some initial changes for. Can you update your patch so it will work with these changes? Also, can you add upgrade database coed to handle %20 --> _ issues in names of wiki articles.

Best, Chris

Hi Professor,

The upgrade_functions code will just read the default.db and inserts the Page items. I have already taken care of this by updating the default.db with the new "_" changes.

I will checkout your changes and will test out my changes after integration. i will create another patch as soon as possible.

Thanks Eswara.

Hi Professor,

Please see cs298_v15.patch

I Pulled latest changes pertaining to doMessage removal initiated by you, Merged and resolved conflicts manually.

Changes summary below:

• Execute Wiki help back logic only-> c=admin or group &&a=wiki && arg= edit or read
• Move Help Javascript source to a new file. Load the help.js only when atleast one button is rendered on the page.
• Fixes help issues that were caused by the latets changes to Pagetitle.
• Clean $_REQUEST params before using.
• Populate help articles in create db by directly, remove logic that fetches pages from sqlite db.
• Added new default.db that upgrade functions use to populate help pages.

Thanks Eswara

Hey Eswara,

This still has the setupWikiHelpBackLogic in controller.php that I complained about. This is called on every page -- even backend pages connected to the crawler that couldn't have help. Push this code into a social component that is used by the admin and the group controllers. Also, $_REQUEST["back_params"] is still used in an unclean fashion. When I applied the patch, the help files still don't show because of the %20 to _ issue. You need to create a new upgrade function to handle this, you can't rely on existing ones that may have already been applied. I am going to push this patch because it addresses some of the issues I asked, but you need to make a new issue addressing the two things mentioned above.

Chris

Hi Professor,

• Sorry for the misunderstanding, I thought it would be sufficient to check for the args/actions for executing the wikiHelpBackLogic . I understand now that you want it to be removed completely from the controller.php.Iw ill try to re-factor the wikiHelpBackLogic in such a way that its removed from controller.php and will be specified only in the controllers required. Is is ok, If the wikihelpBackLogic method is left in controller.php but only called from admin_controller and groups _controller?

please let me know.

• For the %20 to _ issue, Should I create a new function for version 26? Currently the the installations on version 25 are the only ones affected. Should I write a script that re-imports the wiki articles instead? Please advise.

Eswara.

Please remove setupWikiHelpBackLogic from controller.php. You could put it in either in social_component or make a new help component for it (the latter might be more flexible). It could then be called using the call() method in controller.

For the %20 to _ issue, you can make a new version 26 of the db.

Hi Professor,

Thanks for your feedback, I will create a new patch by tonight.

Eswara.

Hi Professor,

I added another patch. To keep things clean, I have added the patch in the new issue I opened for the problems you reported. Please see : http://www.seekquarry.com/mantis/view.php?id=157#c423

You can close this issue if you wish. Then future issues can be tracked in issue# 157.

Thanks Eswara.

Fixed by 6d14e11f2

Attachments:

cs298.patch

cs298_v2.patch

cs298_v3.patch

cs298_v5.patch

cs298_v4.patch

cs298_v6.patch

cs298_v6.1.patch

cs298_v7.patch

cs298_v8.patch

cs298_v9.patch

cs298_v10.patch

ca298_v11.patch

cs298_v12.patch

cs298_v13.patch

cs298_v14.patch

cs298_v15.patch