Forum OpenACS Q&A: Chat package HEAD branch

Collapse
Posted by Pablo Muñoz on
Hi all,

We are happy to announce from Telefónica I+D that we have made an update of the chat package created by Gustaf Neumann, in the AJAX version, improving it with some functionalities.

We are going to upload to the cvs in the HEAD BRANCH, because the UI has changed and maybe someone wants to use the last version.

The current chat has new functionalities like:

• Administrators and teachers can create rooms within a community, class, department, or directly in the main site.
• Much information in the process of creation a new chat room, like end date of the room, maximal number of users, alias, etc.
• System informs new chat room created via email to the users.
• System creates RSS briefs of the conversations in the rooms.
• System recognizes messages with URLs.
• Users can register in a chat room.
• Users can consult chat rooms (open/close and private/public).
• Users can syndicate and consult into RSS briefs.
• Users can receive briefs of the conversations by mail.
• Users can search a room.
• Users can search a transcription.
• Users can send files.
• Users can create private rooms.
• Portlet and index including much information.
• Etc.

Whoever wants to check it can do with the next instance:

http://strauss.gast.it.uc3m.es/

The test option is OpenACS/.LRN PostgreSQL stable (5.3).

You can use the chat like an administrator, and also check the functionalities creating new users as students or teachers.

The code of the chat, if someone wants to download and install it, is in the next url:

http://e-lane.org/public/?folder%5fid=78480

Collapse
Posted by Gustaf Neumann on
Dear Pablo,

With your changes to xotcl-core, you broke a couple of applications here. Furthermore you introduced a hidden dependency to xotcl-core requiring now the chat package. By your changes to the methods of the class ::xo::Chat you added database operations for the tables, which are created by the chat package. For a code maintenance point of few, these operations should be as well in the chat package. There is as well no need for defining e.g. a render2 in addition to render, if one uses the oo abtractions.

Alltogether, i don't see a strong need from your changes for altering the base class ::xo::Chat at all, since chat has already a specialized subclass named ::chat::Chat (which you are using as well).

  ::xo::Chat
     / \
      |

  ::chat::Chat

if you need a special render, add it to
::chat::Chat (no need to name it render2; btw., why does render2 need a different interface (with non-positonal argument -chat_id, were every chat instance has the chat_id as instance variable). There is now a lot of duplicated code in render2 of ::chat::Chat and ::xo::Chat that you have added. Furthermore, render2 of ::chat::Chat never calls render2 of ::xo::Chat, so i fail to see why this is needed. Code duplications makes maintenance a nightmare and is a bad practice in general.

If you need additional actions to be happending, when a user sends a message (or enters the chat etc.), provide your additional functionality e.g. in the method add_msg of class ::chat::Chat and call from this method the method add_msg of ::xo::Chat via next at an appropriate place. If you need an additional interface in ::xo::Chat, i would prefer if you talk to me about this.

By using the already existing subclass one has a lot of freedom and can easily use the methods of the base class. If one needs for some purposes an even more specialized class, on can maybe subclass ::chat::Chat further and just implement what's different in the specialized case there. So one can be sure, not break existing code. Code duplication is kept to a minimum. There is no need to change the base class for every different requirement.

I would be glad to offer you some help for extending the base-class if this is necessary Aand if you want. Since xotcl-core lives only in head, and i want to keep the existing installations working, i'have rolled back your changes to xotcl-core in cvs head for now.

Collapse
Posted by Don Baccus on
Also, there are no upgrade scripts for postgres.

This change needs to be rolled back and re-worked before we accept it.

Collapse
Posted by Don Baccus on
The feature set is nice, it would be good if you can clean things up so we can accept the changes.

Chat's supplied as part of dotlrn, so we have some minimum requirements for code quality, though.

1. Upgrade scripts MUST be provided.

2. You can't break other packages.

3. You can NOT make a generalized service package like xotcl-core depend upon an application like chat. NEVER.

4. Problems like those pointed out by Gustaf should really be cleaned up. Remember, this is open source. People the world over look at your code! If it sucks, people will know.

Collapse
Posted by Raúl Morales Hidalgo on
It also would be nice to provide Oracle support to the new features, or at least good documentation of changes for others to step ahead
Collapse
Posted by Pablo Muñoz on
I've been working on Gustav comments and solving some errors. I will try to correct everything, but I am not sure I can modify everything. Thanks for your comments.
Collapse
Posted by Pablo Muñoz on
Hi again,

I have been reworking on the chat package. The code is now independent of xotcl-core package. The functionalities are the same, but everything is implemented as Gustaf wanted in the chat package. The upgrade scripts for postgres are also implemented.

You can download the code from:

http://e-lane.org/public/?folder%5fid=78480

I expect some comments about the modifications. If everybody agrees now, I will commit to the cvs again.

Best regards.

What about oracle porting?

Once done for a DB is not that hard to do it for the other and that way you don't single out all oracle users.

Collapse
Posted by Emmanuelle Raffenne on
Hi Pablo,

Since your changes on xotcl-core have been rolled back, the chat package in HEAD is now broken. So I would suggest to do 1 of the 2 following: roll back your previous commit on HEAD, or, commit your fixes into HEAD. The latest might make easier to get and test your code.

Collapse
Posted by Pablo Muñoz on
Hi Emmanuelle,

the new version without dependencies is now in the cvs. You can download it from the head branch.

Regards.

Collapse
Posted by Emmanuelle Raffenne on
Thanks Pablo.

I've checked it out from the repository to test it. I got errors because packages/chat/tcl/chat-ajax-procs.tcl has been committed with unresolved conflicts. Please, can you review it and resolve the remaining conflicts?

Collapse
Posted by Pablo Muñoz on
Hi Emmanuelle,

I changed the file with errors by the corrected file, but I am sorry because I did not notice that there were some conflicts.

Now, I guess the conflicts are solved.

Regards.

Collapse
Posted by Malte Sussdorff on
Sorry for being a little bit harsh, but the changes you introduced were bad from a development point of view.

a) Do not make a package dependent of .LRN if it was not so before. There exist "if [apm_package_enabled_p dotlrn]" statements to make sure you can do that without harming non .LRN users

b) If you insert the community name into a room, do not make a query for the name in the __new function, but instead pass it as a parameter so people could actually decide for themselves in what context to name the room.

c) Please remember the naming conventions. It is chat_rooms_private__new (so __ in front of the action) not, chat_rooms__private_new. I did not change that.

d) Please provide language keys in English. I am aware that probably your application is written in spanish, but the main language of the toolkit is English, therefore having at least minimal language keys to know what is going on is helpful.

e) If you need to upgrade, upgrade the .info file as well with the version number so the upgrade can actually happen.

f) If you do not enable RSS Service as a user, do not execute the RSS creation statements when adding a room.

g) ad_schedule_proc is not being stored in the database, therefore if you call ad_schedule_proc and expect the rooms to stay open beyond a restart, that does not work.

h) You depend on chat-portlet in the message keys. Please do not do this. And if you want to do it, make it work the other way round so chat-portlet message keys work with the chat message keys.

j) If you need to set an id (like folder_id) user db_string, not db_1row to check how many there might be and then setting only one.

k) There are other ways to figure out a file storage folder of a community without the need to go over the title in acs-objects. Furthermore, why would you use just this one folder in the first place (chat.tcl)

This is only in three files, but i have given up now. It does not make any sense for me to try to get this to work outside .LRN and to fix in my opinion fundamental errors in development style. Please take a look at how other packages are written and try to make it work without .LRN.

Collapse
Posted by Malte Sussdorff on
Does anyone have a working version of the chat package before the changes introduced by telefonica? They made it in to oacs-5-3 as well, but I think there were some other improvements by gustaf and peter, which are useful. Anyone, anywhere ?
Collapse
Posted by Malte Sussdorff on
Never mind, found it on the oacs-5-2 branch. For the sake of a stable release, could we either make sure that oacs-5-3 chat requires .LRN or that you copy the version from oacs-5-2 over there?

Sorry for being harsh on you and I have absolute faith that you will in the end provide a decent enhancement to the chat package, but knowing the beauty which resides in oacs-5-2 and seeing the major troubles created in oacs-5-3 and HEAD I strongly encourage you to rework the commits if you have the time. I hope my comments so far as well as the commit done will get you started in this direction.

Collapse
Posted by Emmanuelle Raffenne on
Malte,

For now I've added a dependency to dotlrn on oacs-5-3 branch.

When we branched dotlrn we didn't realize that chat package had changed since the oacs-5-2 version (my bad, sorry). I'll add this item to the .lrn meeting agenda to discuss what to do about it.

Collapse
Posted by Malte Sussdorff on
Emmanuelle, just to make sure, my comments were directed in the direction or Telefonica, not you. To my knowledge none of your commits has done anything and I do have the slight suspicion that commits to 5.3 have not been properly rolled back after the decision to commit the changes to HEAD, but I did not want to revert that either.

In my opinion chat should not be dependent on .LRN. If it becomes per design, please make sure to fork it and give it a special name, as I love the package but do not have .LRN installed all the time.

Collapse
Posted by Pablo Muñoz on
Malte,

I am now working on my own, Telefonica is not still working on this project, because as you do not use my name, only the name of the company, I explain to you that. When I posted my first comment Telefonica finished his work. The changes Gustaf told me to do, did that on my own.

I agree with you in the two points of functionality bad behavior (f and g). I had it in my personal improvements of functionality.

I will try to change the code in order to carry out the naming conventions. When I started to work I did not understand anything, and I did not know the naming conventions, so I am sorry.

At least, I hope you like the functionality added.

Regards.

Collapse
Posted by Emmanuelle Raffenne on
Malte,

Yes I understood. But still, my bad has been to branch without checking the state of the package...

Thanks for testing and catching all those issues. As I said in my previous post, we'll discuss it at the .lrn meeting today. If you can join, please do (16:00 gmt).

Collapse
Posted by Don Baccus on
At today's .LRN leadership team meeting we made the following decision:

1. Avni Khatri will revert chat on the oacs-5-3 branch to the oacs-5-2 version, which works. When we merge to HEAD, it will be with the reverted version, so HEAD will also be oacs-5-2 at that point.

2. Before doing so, Avni will tag the existing code so these changes can be retrieved and fixed if someone has time to do so.

3. If someone wants to fix that broken code, they will need to discuss their plans BEFORE doing it and recommitting fixes. Malte's list is quite thorough, and points out many problems with the code that are unacceptable. We're not going to accept new chat code with problems like this.

Collapse
Posted by Pablo Muñoz on
I have a question I would like to ask you. I am finishing with the dotlrn dependency of my chat version, but, in the process of sending files, I have a problem.
When a user send a file within a room, the file is uploaded to the public files of the community which the room belongs to. If the room does not belong to any community, the file is uploaded to the public files of the user.
The current chat version uses file-storage with dotlrn installed, and there is a “public community files” folder, and also a “public user files” folder in the file-storage package. If the chat cannot have dotlrn dependency, could have only file-storage dependency??. The process of send a file would be more complicated. I think it would be necessary to create in the file-storage the folder for the public files, and so on. Would be any problem of have file-storage dependency??

Malte, you told me something about the language, but I do not know what you exactly mean. I have been reading the catalog of the chat, and did not found Spanish keys. You mean names of variables, or what do you mean?
Another point you asked me is why I used a folder and the acs_objects table. I explained at the beginning that in order to upload the files I used the public files of the communities or the users. I did not found another way of getting these folders, and used the acs_objects table, if you know another way, please tell me and will use it.

The chat-portlet catalog is now not used in the chat package, I have changed it, and will try to correct the naming conventions.

The RSS problems are completely truth, it is important to improve it.

Regards.

Collapse
Posted by Malte Sussdorff on
Pablo, first of all thanks for taking on that work. I did not know that the company is not behind the efforts anymore, so thanks for letting me know.

As for your questions:

You should attach the files to the room. You do not need file storage for this, just use a small page that allows you to view all files attached to a room. There exist a couple of these throughout the toolkit, DaveB can point you probably to the best example.
If .LRN is installed, you might copy the files over to the public folder of the community, so on the page where you can see all files attached to the room (cr_item parent_id = room_id) you should have a button "copy" to "select box of all folders in the .LRN communities file-storage". This way you can make files send in chat available in any folder of the community manually and do not have the problem of figuring out what the public folder is.

Language: A lot of language keys in English have been missing. My assumption was that you developed the package in Spanish, which would explain the missing language keys. Otherwise just make sure that you export all language keys before committing.

Hope that helps

Collapse
Posted by Avni Khatri on
Pablo -

Thank you for all the new features you are adding to chat.
I know a lot of the groups that use OpenACS/.LRN can utilize these features.

Your current commits, however, are causing a lot of errors/problems. We are attempting to get a release out in early-mid July and a lot of these same groups depend on the chat being completely functional.

As Don mentioned - I am going to revert your changes thus far on
the 5-3 branch back to 5-2. These changes will then be merged into HEAD after the release so we have a functioning chat in the distribution.

I am going to do the reverting Saturday (6/30) afternoon PDT.

I don't want to lose all the work you've done so far though. When I revert, I will make a note that I am reverting, so we can introduce your changes with fixes at a later date. I will help you re-commit your changes if you have time to work with me. We will have to work with the OCT and the .LRN leadership team to make sure each feature/change and code-set is approved before we re-commit though.

Since we are reverting, hang tight on committing any more changes right now. Contact me directly via email at avni321 at yahoo dot com. Thanks for your patience.

Avni

Collapse
Posted by Avni Khatri on
Pablo and everyone -

I am still working on reverting the chat changes. Planning to finish tonight.
Ran into some CVS issues which Emmanuelle helped me resolve.

Thanks,
Avni