Description

Integral Community Exchange System (IntegralCES or ICES in short) is a suite of modules featuring social currencies (also called local, alternative or complemntary currencies) management for communities. It is an open-source alternative to the widely used but now outdated Community Exchange System.

Concretely IntegralCES is a set of different modules for different specific features:

  • ces_bank: It is the main module: the banking or accounting module. It supports different currencies and exchanges between them. It features account debit and credit limits and exchange administration. A single user can have one or more accounts in the different exchanges.
  • ces_summaryblock: It is a simple dupal block resuming your state in the exchange network.
  • ces_offerswants: It is a very simple offers and wants module integrated to the banking module.
  • ces_blog: It is a very simple module featuring a dedicated blog for each exchange.
  • ces_statistics: It offers statistical views for data from ces_bank module.
  • ces_notify: It helps with the messages due to transactions or other events.
  • ces_user: It adds some fields to the drupal user entity needed to identify the seller or buyer within the exchange community.
  • ces_import4ces: Imports data from the CES software, from the CSV files it provides.

This project is a community effort from the Cooperativa Integral Catalana and Ecoxarxes from Catalonia (Spain), a federated set of alternative currency communities.

Links

Drupal project page: https://drupal.org/sandbox/esteve/1367140
Source code: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/esteve/1367140.git ices
Site page: https://www.integralces.net
Demo page: https://demo.integralces.net
Code review: http://pareview.sh/pareview/httpgitdrupalorgsandboxesteve1367140git

Observations

  • This is a somewhat big and complex project. If you are interested in this project and you want to help us having a better and bug-free code, you are very welcome! If you just want to post the output of the coder module and get a bonus point for your project to be reviewed, please don't waste your and our precious time :)
  • About pareview.sh: We have not finded in any documentation that the names of sub-modules have to be prefixed with the name of the parent module. We think they are simply false positives in pareview.
  • We specially need the project to be a full one to be able to benefit from the drupal.org translation and continuous integration services. Also it would be easier to install for new communities if it becomes full project.
  • This project is currently in production at www.integralces.net with a few thousands of users.
CommentFileSizeAuthor
#11 ices_codesniffer_results.txt4.48 KBrob.barnett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented
Issue summary: View changes
Botto’s picture

Botto CreditAttribution: Botto commented
  1. Currently it's on a master branch, it's should ideally be on a major version branch https://drupal.org/node/1127732
  2. Go through par review http://pareview.sh/pareview/httpgitdrupalorgsandboxesteve1367140git and correct all the errors it brought up
  3. Not sure what the ces_blog brings that you could not already do with other modules
  4. All the jqPlot libraries should ideally be included using libraries API https://drupal.org/project/libraries
PA robot’s picture

PA robot CreditAttribution: PA robot commented

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Commited changes in ces_statistics submodule. Now the jqPlot libraries are included using libraries API. See:
http://drupalcode.org/sandbox/esteve/1367140.git/commit/0c0dcdd
Thanks botto!

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented

Botto:

  1. The master branch was completely empty, now it's deleted :)
  2. The pareview.sh has a lot of output for this project since it's quite big. I'll have to review it carefully. We used the coder module to check the code style and it has almost no issues. I think there are lots of false positives and, anyway, I would like to know whether these warnings are only informative or otherwise some (or all) of them are normative to be able to become a full project. Do you know?
  3. The ces_blog is a very very simple module, with very little code. It creates a new content type with a hidden field: the exchange, and defines the page filtering the content type by this new field and adds a pair of navigation menu links. All this probably could be done with configuration and with a view (and with a little glue code anyway in order to set the proper value to the field), but what we are doing more or less is to write that configuration and view in the code, in order to give a very precise feature and avoiding installation issues. I don't think this is repeating code since we are not doing any generic logic but using the drupal APIs to add that bit of functionality to what the node module already do. Anyway, me may have missed an even simpler way to achieve the same result (without having to rewrite the whole project please), so we are obviously open to suggestions.
  4. Thanks XaviP for fixing it... But this means that now our project depends on the libraries project, right? I'm not sure if this solution is better or worse than before, because we added a big dependency line only to add some JS files. But its ok for me.

Thanks @Botto for take a look into the code!

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Hi, esteve.badia
No dependencies are needed. I code it with hook_library and drupal_add_library, which are both in core:
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ad...
We can see in http://www.jqplot.com/info.php that jqPlot is GPL2 licensed:

jqPlot is an open source project dual licensed under the MIT and GPL version 2 licenses. You are free to choose the license that best suits your project.

In statistics module, the jqPlot's url is in code, as a recognition.
I hope everybody is happy with this solution.

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented

Ok, I misunderstood the solution about the libraries! Thanks again!

Botto’s picture

Botto CreditAttribution: Botto commented

The pareview.sh mostly looks like just coding standards. Ignoring the top half complaining about the module naming.
You can get a lot of the same output using https://drupal.org/project/drupalpractice which will be better for local debugging.

edumag’s picture

edumag CreditAttribution: edumag commented

Corrected problems indicating pareview

Now we have a good clean code on branch 7.x-1.x

We have separate libraries jqplot and added instructions to install.

Thanks to our friend pareview

rob.barnett’s picture

rob.barnett CreditAttribution: rob.barnett commented

esteve.badia,

This looks like a great suite of modules. Some issues I found:

Automated Review:
You should run your code through Code Sniffer. It found several errors and warnings (see attached).

Manual Review:
You may want to use module_load_include() in place of require_once()
In ices.module ices_preprocess_page() why do you do have global $user and then $user = user_load($user->uid)?
In your install file you should uninstall all variables as well.
Consider adding configure to your .info for admin pages.
Your javascript should use Drupal.behaviors.behavior_name instead of $(document).ready(function()

rob.barnett’s picture

rob.barnett CreditAttribution: rob.barnett commented
FileSize
ices_codesniffer_results.txt4.48 KB

I forgot to include the codesniffer results (attached).

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Two hurley's issues fixed:
Duplicate call to get $user in ices_preprocess_page()
Javascript should use Drupal.behaviors in ces_statistics

davidam’s picture

davidam CreditAttribution: davidam commented

Hello,

I've a fresh installation from git. I've added two posts to the blog, but the message is "There are no blog posts yet.".

Thanks for the efforts.

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Hi davidam,
Have you created a new exchange? If not, please follow the ices help link in modules list to proceed.
The blog is a submodule you have to enable and every blog is specific for each exchange group, so each exchange group administrator can access to create blog posts of the excange through the navigation menu (Administration>Post)
You can see an example logining as administrator (user Riemann or Fermat) in the demo site:
https://demo.integralces.net
Please, feedback if this is not a good enough explanation, or if it doesn't work as dessigned.
Thanks

davidam’s picture

davidam CreditAttribution: davidam commented

I've all submodules enables. Now, I've created a new exchange, then I create a new post and the result is the same "There are no blog posts yet."

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Hi davidam,
I've created a better explanation in ices help for creating the first exchange group

- Visit new exchange form (ces/bank/exchange/new) and initialize the first exchange group. You must fill in the Administrator section with the user that will be the exchange administrator (It cannot be the current drupal admin).
- Visit admin exchanges page (ces/admin/ces) , click on the new exchange and activate it.
- Login as exchange administrator and write some welcome blog posts.

(issue https://www.drupal.org/node/2332209)
Thank you!

davidam’s picture

davidam CreditAttribution: davidam commented

I've installed ices again, I've found an error

The following extensions will be enabled: ces_bank, ces_blog, ces_develop, ces_import4ces, ces_interop, ces_message, ces_offerswants, ces_statistics, ces_summaryblock, ces_user, ices, rest_server, services, xautoload, oauth2_server
Do you really want to continue? (y/n): y
PHP Fatal error: Call to undefined method i18n_object_wrapper::strings_update() in /var/www/orgmode/sites/all/modules/contrib/i18n/i18n_string/i18n_string.module on line 851

You must include i18n_string as dependency in some submodule, or in ices.info.

In the second step Visit admin exchanges page:

Notice: Trying to get property of non-object en email_example_mail_alter() (línea 138 de /var/www/orgmode/sites/all/modules/contrib/examples/email_example/email_example.module).
Notice: Trying to get property of non-object en email_example_mail_alter() (línea 138 de /var/www/orgmode/sites/all/modules/contrib/examples/email_example/email_example.module).
Notice: Trying to get property of non-object en email_example_mail_alter() (línea 138 de /var/www/orgmode/sites/all/modules/contrib/examples/email_example/email_example.module).

Finally, trying node/add found a white screen of death. Disabling ices node/add is ok.

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Hi davidam,

IntegralCES doesn't need i18n nor example as dependencies.
Maybe your drupal core installation is not clean? Or maybe cleaning cache is necessary?
Look where your errors point:
sites/all/modules/contrib/i18n
sites/all/modules/contrib/examples
These are not ices module errors. Sorry.

About accessing to node/add, we've reopen this issue:
https://www.drupal.org/node/2208065
It seems a drupal bug (https://www.drupal.org/node/647064), but we will review again to be sure.

HongPong’s picture

HongPong CreditAttribution: HongPong commented

I think the module is pretty well ready to go into full module status, if code linter is reasonably clean. I've seen plenty of modules approved that were essentially stubs, whereas this one is already live in the world. If #647064: Fatal error: Unsupported operand types when menu_links query returns no results in system_admin_menu_block() related longstanding bug is causing the issue #2208065: Error accessing /node/add, perhaps it could be caught better. However I don't think there are actually any blockers to a full approval right now. Thanks for all the help refining this important project everyone :)

XaviP’s picture

XaviP CreditAttribution: XaviP commented

Issue #2208065 Error accessing /node/add solved

XaviP’s picture

XaviP CreditAttribution: XaviP commented

As commented in #10 by hurley, added config in ces_bank.info to administer exchanges and accounts:
Issue #2345469: Adding configure to ces_bank.info for admin exchanges

davidam’s picture

davidam CreditAttribution: davidam commented

Another task is improve the README with the template: https://www.drupal.org/node/2181737

XaviP’s picture

XaviP CreditAttribution: XaviP commented

davidam,
There's an entire folder (/docs) with documentation (doxygen). We are working on translations.
Also, you can see all this documentation in https://docs.integralces.net/dox
salut!

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented
Issue summary: View changes
esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented
Issue summary: View changes
davidam’s picture

davidam CreditAttribution: davidam commented
Status: Needs review » Needs work

Now, I've a little more time to make the review.

Automated Review

There are a lot of errors for pareview.sh today:
http://pareview.sh/pareview/httpgitdrupalorgsandboxesteve1367140git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Follows the guidelines for individual user accounts.

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
No: IMHO you must add LICENSE.txt. See Licensing FAQ.

3rd party code
Yes: Follows the guidelines for 3rd party code.

Coding style & Drupal API usage
1. The CES setting could be linked from admin/config.
2. The url ces/bank/exchange/limitchain is not directly editable by admin.
3. The url ces/admin/account/new and ces/admin/limit/new returns a white page.
4. In ces/admin/ces could appears a link to add exchange requests

PA robot’s picture

PA robot CreditAttribution: PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

XaviP’s picture

XaviP CreditAttribution: XaviP commented
Issue summary: View changes
Status: Closed (won't fix) » Needs review
XaviP’s picture

XaviP CreditAttribution: XaviP commented
Issue summary: View changes
XaviP’s picture

XaviP CreditAttribution: XaviP commented
Issue summary: View changes
matslats’s picture

matslats CreditAttribution: matslats commented
Status: Needs review » Reviewed & tested by the community

I'm familiar with this project though I haven't looked at the code recently. It has a few man-years work done already, and it needs to be a full project.

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented

Thanks @matslats, I appreciate very much your trust in the project!

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented

@esteve.badia: We can promote this single project manually to a full project for you if that's what you would prefer. Otherwise this queue is for granting users "git vetted user" status - can you give some direction on which code we can evaluate for you? We cannot grant "git vetted user" status based on contributions by others.

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented

@kscheirer: You can check, for example, the submodule ces_interop inside this ices project, that has been exclusively written by me (It can be verified via git). I have no other Drupal public projects besides "ices" and the ad-hoc theme for this project "greences", which has also contributions from others.

But anyway it is ok if you can manually promote this single project, since this is what I am (and the small community supporting the project are) most interested in.

Thanks!

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented
Assigned: Unassigned » klausi
Status: Postponed (maintainer needs more info) » Reviewed & tested by the community

Review for ces_interop

  • You have a typo in the info file, "Impleemnts" should be "Implements"
  • In ces_interop_opentransact() you're saving POST values to a global, can you add some filtering there? I'm not sure how that global is used yet, but generally user input is not to be trusted
  • The docblock for ces_interop_form_oauth2_server_authorize_form_alter() is wrong, this is actually an implementation of hook_form_FORM_ID_alter()
  • In ces_interop_entity_presave() it looks like you're reading globals to save in the entity. I think you're trying to persist values in a single PHP session, using drupal_static() would work for that here
  • ces_interop_admin() is not an implementation of hook_admin, that hook does not exist

Looks pretty good to me. Assigning to klausi as he may have time for a final review.

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented

@kscheirer: Thanks for the comments!

  • I've fixed the texts in (1), (3) and (5).
  • I have not cleaned the variable in (2), since it does not represent any danger. The later use is safe and additionally, if the value is invalid, the authorization module aborts the execution so my module never uses it. I've however put a comment. I could add a check there (the value should be alpha_numeric), but I prefer that is the oauth2_module who do the check because then it will return the right OAuth2 error.
  • As you noted (4), I was using a global variable to save a value from the query (an oauth2 request) to be able to use it later inside a hook (after giving the control to oauth2 module). I've changed this strategy to the drupal_static one, which indeed seems cleaner.
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Assigned: klausi » Unassigned

@kscheirer: ces_interop_opentransact(): Since the $_POST values are not printed to HTML here I see no XSS vulnerability there, so that usage should be fine.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented
Status: Reviewed & tested by the community » Fixed

Thanks for the extra review klausi!

Thanks for your contribution, esteve.badia!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

esteve.badia’s picture

esteve.badia CreditAttribution: esteve.badia commented

We are very grateful to all reviewers for your time, insights and confidence, from me and the other folks in the development team :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.