Hi Professor,

Please find the patch attached with all the updates below:

- Added PhantomJS tests. currently the tests are run using the user account root/"" . please let me know If you want me to move this information to a config file somewhere. - Added Delete confirmation to more places in Yioop. - Changes the expand/collapse icons for the feeds& wikis page and Wiki page. - Added an icon for Start new thread link in Recent Group Activity , with a title attribute. - I tried to do a "show expand icon on hover" but was not able to get it to work with empty contents inside the div, which holds the icon.

[Edit] Instructions to run PhantomJS

• Make sure PhantomJS is installed. You can download a copy from : http://phantomjs.org/download.html

• PhantomJs is available for Mac,Windows and Linux.

• Makse sure "phantomjs" is added to the PATH system variable. The Yioop PhantomRunner assumes a standard phantomJS installation and expects "phantomjs" to be added to the PATH.

• If phantomjs is not added to the path, you have the option to pass a custom path to phantomJS in the /tests/phantom.php, by passing the "path/to/phantomjs" as a parameter to the YioopPhantomRunner constructor.

example : $yioop_phantom_runner = new YioopPhantomRunner("path/to/phantomjs");

• Once Phantomjs is setup, you should be able to run the tests from : tests/?activity=runTestBasedOnRequest&test=phantomjs_ui

• Also, If you wish, you can run the PhantomJs javascript tests directly by running $ phantomjs /tests/web_ui_tests.js (optional "true" can follow to print debug statements)

$ phantomjs /tests/mobile_ui_tests.js

Thanks Eswara

Hey Eswara,

The patch looks mainly good. I am applying it and pushing it live. Here are things I would like in a new patch. (1)Please add to this issue instructions on setting up phantomjs. (2) Also, the title attribute tooltip Start New Thread should maybe say Start New Thread in [Name of Group]. Then when you click it the form that opens should have as its title Start New Thread in [Name of Group]. Right now the title of the form is Start New Thread. (3) Start New Thread seems somewhat awkwardly aligned as it appears to the right of the person's icon rather than beneath it. On the other hand, the rest of the form is beneath the icon. Try to reformat this.

Best, Chris

Hi Professor,

Thanks for the feedback. I will make the required changes and will create another patch.

Thanks Eswara

Hi Professor,

Please find cs298_enhancements_v1.patch. I have made the changes you suggested , please let me know if you have any concerns.

Thanks Eswara

Your patch didn't seem to handle localization properly. Please use the argument form of tl() to do the localization rather than concatenate on the group name. That might make sense for English but not necessarily for other languages. I.e., You should use tl('label_name', $value); and have translations like: Here is where %s the variable goes.

Hi Professor,

I initially tried to use "%s" however I couldn't use it for populating the group_name inside the javascript function - start_thread_form. I had to pass the group_name as a javascript param as the function gets written by PHP only once.

there are 2 places where I used the "Start New Thread in" in the commit. I can surely add another localization variable with %s for the place where I display the title attribute(as its in a loop) however please advise me how I can use the variable with %s while painting the Javascript function.

Eswara

I think Shawn Tice had to do something like this a few places. For example, line 204 classifier.js. In any case, there were no English values for the new localization strings in the patch as given when I tested it.

Hi Professor,

Sorry for the localization vars missing, I realized I attached an older version of the patch. I have implemented the localization var 'groupfeed_element_start_thread_in_group' with %s, Please take a look at the patch : cs298_enhancements_v2.patch

Please let me know If you have any concerns.

Thanks Eswara

Hi Eswara,

(1)The phantom js stuff is not adequately documented. There are lots of methods in YioopPhantomRunner and phantom.php that have no documentation. The documentation that exists is not saying more than what could already be gleaned from the names of files and methods. (2) The two files YioopPhantomRunner (which does not follow Yioop file name conventions) and phantom.php should be combined into just phantom.php (3) I don't like adding additional entry points into Yioop. If you create a service, it needs to go through an index.php file not a stand-alone set-up like phantom.php. I want to easily be able to audit attack vectors on Yioop. (4) Non-subclasses of one of the unit test classes or experiment classes should not be in the tests folder. So phantom.php should be refactored, some code going into a file in the lib folder, some code should go into tests/index.php (5) web_ui_tests.js has little documentation, lots of long lines, etc. It needs to conform to Yioop coding guidelines for Javascript. (6) Javascript methods even in files like phantomjs_uitest.php should be documented.

Chris

Hi Eswara,

In addition to the above, I still can't the phantomjs code to work. I have phantomjs installed as per the instructions. So I am backing out the phantomjs stuff for now. I also noticed that help.js is using deprecated events and needs to fixed.

Chris

Hi Professor,

I have made a lot of changes and refactored the phantomJS code. Please find the patch attached.

--> phantomjs_refactor.patch

Please let me know your comments. Also can you please point me the issues you saw in help.js. I am not sure which are the deprecated events in help.js. [edit] added minor code change for phantomjs binary detection.

Thanks Eswara

Hi Professor,

I have made the changes you have suggested, also made minor changes to help articles and a message on help article retreival failure.

Please find phantomjs_refactor_v1.patch atatched.

Let me know if you have any concerns. Sorry for the delay, I was catching up with work on Friday.

[Edit] Minor formatting changes.

Thanks Eswara.

Hey Eswara,

One thing I noticed is that if you add a new help location, but the help file doesn't exist yet, then it is impossible to edit from the UI this new location. Could you let the user do that?

Chris

Hey Eswara,

I was noticing also that your javascript wiki parser is a little too agressive in starting new paragraphs. It should only start a new paragraph after two new lines. In particular, if I have a bulleted item:

• some paragraph continued here

This should all be within one li tag after parsing. Can you revise your patch to handle the above two things? I have pushed some changes to the create file that conflicted with your stuff so I need a new patch anyway. Notice in createdb file you are not allowed to have lines more than 80 chars either.

Best, Chris

Hi Professor,

Thanks for your comments. I will make the changes you suggested and will send another patch ober your latest commit.

Thanks Eswara

Hi Professor,

I am done with all the changes except the change with new paragraphs. The reason why I am inserting a new paragraph on single line break is by following the mediawiki syntax. Mediawiki also inserts a new paragraph on 1 line break. Can you confirm if you want me to change the behaviour against Mediawiki? Please see the screenshot attached.

[Edit] If you want to discuss more on tuesday, I will hold off on that for now and I can attach the patch with everything else. Please let me know.

Thanks Eswara

Hi Professor,

Please see phantomjs_refactor_v2.patch. For now this patch contains all the refactoring to phantomjs, all the changes you suggested except the wiki parser "new paragraph" change. If you can take a look at this and apply if its good, I will create a new patch for any parser changes If required.

Eswara.

Hey Eswara,

The patch doesn't apply because of createdb.php and the Telegu configure.ini. Also, the filename YioopPhantomRunner.php does not follow Yioop naming conventions.

Best, Chris

Hold off a sec on anew patch. I delete the conflicts from the old patch and am testing.

Chris

Sure, let me know when you want me to create a new patch. i will pull and create a new patch for my changes.

Thanks Eswara

Hey Eswara,

I'll probably push my modified version of your stuff some time later tonight. (After kids asleep -- so don't hold your breath). Thanks for your code.

Best, Chris

Hi Professor,

Sure, Please apply the patch whenever you can and let me know If you see conflicts. If you do see any, I will wait for you to push and create another patch on top of it.

Thanks Eswara.

Your patch is pushed. Can you do the double newline thing and then we can call it a day? I am using \n\n to separate paragraph and item lists on the server side parser.

Sure Professor. I will work on the double line break to seperate paragraphs and lists. Thanks for pushing the patch.

Eswara

Hi Professor,

Please see attached patch "help_refactor.patch". I made some changes to regex to honor atleast double newlines to add a block , and in turn preserve the old functionality. Sorry for the delay, It was so much pain to make all different patterns to work :). let me know your feedback.

Thanks, Eswara

Hi Professor,

Please see attached patch "help_refactor.patch". I made some changes to regex to honor atleast double newlines to add a block , and in turn preserve the old functionality. Sorry for the delay, It was so much pain to make all different patterns to work :). let me know your feedback.

Thanks, Eswara

Cool, seems to work. Thanks. Will close this issue now.

Fixed by cbecd67

Attachments:

cs298_enhancements.patch

cs298_enhancements_v1.patch

cs298_enhancements_v2.patch

phantomjs_refactor.patch

phantomjs_refactor_v1.patch

phantomjs_refactor_v2.patch

help_refactor.patch