iProjectWeb is a simple project management and task management application. It has a minimalistic set of features necessary for organizing effective project team work.

The application has a tab-based ajax design.

It supports roles of system administrator, project manager and team member with proper object ownership rights distribution

Project, task, and risk attributes are organized as knowledge base allowing treating statuses and priorities in a unified way across several projects and teams.

Risk management allows problem prevention actions planning and motoring.

Simple and easy to use file manager allows linking uploaded files to a particular task or project

Application design allows having separate mailing lists for each particular task.

the links:
git - http://git.drupal.org/sandbox/systemdesigner/1387058.git
the project page: http://drupal.org/sandbox/systemdesigner/1387058

CommentFileSizeAuthor
#4 out.txt112.34 KBnatemow

Comments

natemow’s picture

natemow commented
Status: Needs review » Needs work

Automated review:

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

  • Remove "version" from the info file, it will be added by drupal.org packaging automatically.
  • The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
    ./iprojectweb_appconfigdata.php
./iprojectweb_resources_en.php
./iprojectweb_menu.php
  • Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
    ./views/index.html:                                                  ASCII text, with no line terminators
./index.html:                                                        ASCII text, with no line terminators
./css/index.html:                                                    ASCII text, with no line terminators
./js/tinymce/langs/index.html:                                       ASCII text, with no line terminators
./js/tinymce/plugins/fullscreen/editor_plugin.js:                    ASCII text, with very long lines, with no line terminators
./js/tinymce/plugins/fullscreen/index.html:                          ASCII text, with no line terminators
./js/tinymce/index.html:                                             ASCII text, with no line terminators
./js/tinymce/utils/index.html:                                       ASCII text, with no line terminators
./js/tinymce/tiny_mce.js:                                            ASCII text, with very long lines, with no line terminators
./js/tinymce/themes/advanced/langs/index.html:                       ASCII text, with no line terminators
./js/tinymce/themes/advanced/img/index.html:                         ASCII text, with no line terminators
./js/tinymce/themes/advanced/skins/default/img/index.html:           ASCII text, with no line terminators
./js/tinymce/themes/advanced/skins/default/index.html:               ASCII text, with no line terminators
./js/tinymce/themes/advanced/skins/o2k7/img/index.html:              ASCII text, with no line terminators
./js/tinymce/themes/advanced/skins/o2k7/index.html:                  ASCII text, with no line terminators
./js/tinymce/themes/advanced/index.html:                             ASCII text, with no line terminators
./js/tinymce/themes/advanced/editor_template.js:                     ASCII text, with very long lines, with no line terminators
./js/tinymce/themes/advanced/js/index.html:                          ASCII text, with no line terminators
./js/index.html:                                                     ASCII text, with no line terminators
./js/calendar/lang/index.html:                                       ASCII text, with no line terminators
./js/calendar/index.html:                                            ASCII text, with no line terminators
./js/calendar/css/index.html:                                        ASCII text, with no line terminators
./styles/index.html:                                                 ASCII text, with no line terminators
./styles/std/images/index.html:                                      ASCII text, with no line terminators
./styles/std/index.html:                                             ASCII text, with no line terminators
./styles/std/css/index.html:                                         ASCII text, with no line terminators
  • Remove all old CVS $Id tags, they are not needed anymore.
    js/tinymce/plugins/fullscreen/editor_plugin_src.js:2: * $Id: editor_plugin_src.js 923 2008-09-09 16:45:29Z spocke $
js/tinymce/utils/form_utils.js:2: * $Id: form_utils.js 1184 2009-08-11 11:47:27Z spocke $
js/tinymce/utils/mctabs.js:2: * $Id: mctabs.js 758 2008-03-30 13:53:29Z spocke $
js/tinymce/utils/editable_selects.js:2: * $Id: editable_selects.js 867 2008-06-09 20:33:40Z spocke $
js/tinymce/utils/validate.js:2: * $Id: validate.js 758 2008-03-30 13:53:29Z spocke $
js/tinymce/themes/advanced/editor_template_src.js:2: * $Id: editor_template_src.js 1045 2009-03-04 20:03:18Z spocke $
js/calendar/calendar-setup.js:22:// $Id: calendar-setup.js,v 1.25 2005/03/07 09:51:33 mishoo Exp $
js/calendar/calendar.js:15:// $Id: calendar.js,v 1.51 2005/03/07 16:44:31 mishoo Exp $

    • sed: -e expression #1, char 3: unexpected `,'

  • Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/iprojectweb.module:
 +1: [minor] There should be no trailing spaces
 +4: [minor] indent secondary line of comment one space
 +4: [minor] Format should be * Implements hook_foo().
 +5: [minor] indent secondary line of comment one space
 +9: [minor] There should be no trailing spaces
 +12: [minor] There should be no trailing spaces
 +25: [minor] There should be no trailing spaces
 +28: [minor] There should be no trailing spaces
 +31: [minor] indent secondary line of comment one space
 +32: [minor] indent secondary line of comment one space
 +33: [minor] indent secondary line of comment one space
 +40: [minor] There should be no trailing spaces
 +41: [minor] There should be no trailing spaces
 +45: [minor] There should be no trailing spaces
 +46: [minor] There should be no trailing spaces
 +47: [minor] There should be no trailing spaces
 +50: [minor] There should be no trailing spaces
 +51: [minor] There should be no trailing spaces
 +54: [minor] There should be no trailing spaces
 +55: [minor] There should be no trailing spaces
 +58: [minor] There should be no trailing spaces
 +59: [minor] There should be no trailing spaces
 +60: [minor] There should be no trailing spaces
 +64: [minor] There should be no trailing spaces
 +71: [minor] There should be no trailing spaces
 +74: [minor] There should be no trailing spaces
 +75: [minor] There should be no trailing spaces
 +76: [minor] There should be no trailing spaces
 +77: [minor] There should be no trailing spaces
 +78: [minor] There should be no trailing spaces
 +79: [minor] There should be no trailing spaces
 +81: [minor] There should be no trailing spaces
 +82: [minor] There should be no trailing spaces
 +83: [minor] There should be no trailing spaces
 +84: [minor] There should be no trailing spaces
 +85: [minor] There should be no trailing spaces
 +86: [minor] There should be no trailing spaces
 +87: [minor] There should be no trailing spaces
 +96: [minor] There should be no trailing spaces
 +98: [minor] There should be no trailing spaces
 +117: [minor] There should be no trailing spaces
 +118: [minor] There should be no trailing spaces
 +123: [minor] There should be no trailing spaces
 +127: [minor] There should be no trailing spaces
 +131: [minor] There should be no trailing spaces
 +135: [minor] There should be no trailing spaces
 +136: [minor] There should be no trailing spaces
 +139: [minor] There should be no trailing spaces
 +143: [minor] There should be no trailing spaces
 +144: [minor] There should be no trailing spaces
 +148: [minor] indent secondary line of comment one space
 +148: [minor] Format should be * Implements hook_foo().
 +149: [minor] indent secondary line of comment one space
 +151: [minor] There should be no trailing spaces
 +159: [minor] There should be no trailing spaces

Status Messages:
 Coder found 1 projects, 1 files, 55 minor warnings, 0 warnings were flagged to be ignored
ERROR: the "DrupalCodingStandard" coding standard is not installed. The installed coding standards are MySource, PEAR, Zend, Squiz and PHPCS
systemdesigner’s picture

systemdesigner commented
Status: Needs work » Needs review

Next step ready.

Items from the list above are implemented. a lot of additional work is done.

natemow’s picture

natemow commented

Manual review:

  • js/*: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
  • Remove extraneous index.html files (e.g. css/index.html, views/index.html).
  • Generally, this project seems to use very little of the D7 API -- I wonder if it may be better to move the core classes to a GitHub project or something, then instead create a wrapper module for it?

Automated review: attached below.

natemow’s picture

natemow commented
StatusFileSize
new out.txt112.34 KB
natemow’s picture

natemow commented
Status: Needs review » Needs work
systemdesigner’s picture

systemdesigner commented
Status: Needs work » Needs review

1. Let me comment the latest results I obtained from the auto review system point by point

Review of the 7.x-1.x branch:
The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
./styles/std/getStyle.php

There is no "?>" PHP delimiter at the end of this file

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/iprojectweb_ihtml.php
130 | ERROR | Line indented incorrectly; expected 4 spaces, found 0
140 | ERROR | Line indented incorrectly; expected 4 spaces, found 12
436 | ERROR | Line indented incorrectly; expected 4 spaces, found 0

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/iprojectweb_layout.php
113 | ERROR | Line indented incorrectly; expected 4 spaces, found 0

All these messages are of similar nature: indentation of php code included into html. I believe that the lines are indented correctly in all those places. Anyways any your suggestions are welcome.

FILE: ...ules/pareview_temp/test_candidate/iprojectweb_projects_mailinglists.php
18 | ERROR | Class name must use UpperCamel naming without underscores
FILE: ...all/modules/pareview_temp/test_candidate/iprojectweb_projects_teams.php
18 | ERROR | Class name must use UpperCamel naming without underscores
FILE: ...modules/pareview_temp/test_candidate/iprojectweb_risks_mailinglists.php
18 | ERROR | Class name must use UpperCamel naming without underscores
FILE: ...modules/pareview_temp/test_candidate/iprojectweb_tasks_mailinglists.php
18 | ERROR | Class name must use UpperCamel naming without underscores

These 4 classes (out of 40) are special ones. Underscores in their name say only about their goal (to serve as mtm links between db objects solely). So this is not an intention to break the rules, this is just a notion of their uniqueness.

2. Thank you for your notes about the third-party libraries. All of them are removed from git. Our main intention of including them was just review convenience.

3. Regarding your notes about D7 API. That’s an interesting point to begin a discussion.

There are several design patterns saying about how to split an application into layers making them relatively independent.

The alternative way is to create numerous entry points, one or more for each object, assign a hook_menu() to each of them and dispatch requests in the url – parsing way.

Similar things may be said about database select/update/insert operations, when each object defines its own methods and makes direct calls to db_query or db_insert.

This is good from viewpoint of API calls spreading in the code, but not that good from viewpoint of module maintenance and further development. Isn’t it?

Thank you for your time and valuable comments.

With all respect,
George

natemow’s picture

natemow commented
Status: Needs review » Needs work

Manual review:

Per the getStyle.php flag -- I think the coder review is choking on your closing comment in the @file area. Looking at your .module file, I see a few issues with that require_once statement as well (line 131) ...this is really just a snapshot, though; globally, I think there's quite a few improvements that could be made to take better advantage of the API.

  • This is the preferred method for including files: require_once(drupal_get_path('module', 'yourmodulename') . '/path/to/yourmodulename.inc'); ...note the .inc extension as well.
  • In the case of getStyle.php, you should use drupal_add_css instead.
  • Rather than use a custom function ("iprojectweb_entrypoint" in this case) for setting up your runtime, consider using hook_init instead. Generally speaking, you should use hooks and preprocess functions wherever you can, so other Drupal devs can quickly pick up on where things are happening.

Automated review:

Review of the 7.x-1.x branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.


FILE: ....x/sites/all/modules/pareview_temp/test_candidate/iprojectweb_ihtml.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 130 | ERROR | Line indented incorrectly; expected 4 spaces, found 0
 140 | ERROR | Line indented incorrectly; expected 4 spaces, found 12
 436 | ERROR | Line indented incorrectly; expected 4 spaces, found 0
--------------------------------------------------------------------------------


FILE: ...x/sites/all/modules/pareview_temp/test_candidate/iprojectweb_layout.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 113 | ERROR | Line indented incorrectly; expected 4 spaces, found 0
--------------------------------------------------------------------------------


FILE: ...ules/pareview_temp/test_candidate/iprojectweb_projects_mailinglists.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 18 | ERROR | Class name must use UpperCamel naming without underscores
--------------------------------------------------------------------------------


FILE: ...all/modules/pareview_temp/test_candidate/iprojectweb_projects_teams.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 18 | ERROR | Class name must use UpperCamel naming without underscores
--------------------------------------------------------------------------------


FILE: ...modules/pareview_temp/test_candidate/iprojectweb_risks_mailinglists.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 18 | ERROR | Class name must use UpperCamel naming without underscores
--------------------------------------------------------------------------------


FILE: ...modules/pareview_temp/test_candidate/iprojectweb_tasks_mailinglists.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 18 | ERROR | Class name must use UpperCamel naming without underscores
--------------------------------------------------------------------------------
systemdesigner’s picture

systemdesigner commented
Status: Needs work » Needs review

I have implemented several your suggestions regarding css loading and drupal path in the require_once calls.

Unfortunately it seems that hook_init() never gets called, so I did not manage to wrap the environment initialization with this method.
From other side in the 'official' examples there are places where the content is prepared by a custom function, so I do not see a major issue here.

Meantime, do you have any comments to my comments (#6) to the automated review results?

systemdesigner’s picture

systemdesigner commented
Status: Needs review » Needs work
systemdesigner’s picture

systemdesigner commented
Status: Needs work » Needs review

the .inc extension is applied for all the files where it is necessary. Please also look at my post #8.

systemdesigner’s picture

systemdesigner commented
Priority: Normal » Major
peterx’s picture

peterx commented

The Storm project appears to be your major competitor. How does your project compare? When I am next starting a new Drupal based project, why should I use your project instead of Storm?

Risk management? What is the risk you manage? How? You could use a screen shot or demo page to show what you mean.

"motoring"? Do you mean monitoring?

KhaledBlah’s picture

KhaledBlah commented
Status: Needs review » Needs work

First of all, thanks for your contribution!

Automatic review of branch 7.x-1.x:

please have a look at http://ventral.org/pareview/httpgitdrupalorgsandboxsystemdesigner1387058git. The automatic review listed issues in your 7.x-1.x branch.

Manual review of branch 7.x-1.x:

  1. I am not sure what kind of status the master branch is in but I understand from other reviews here at d.o that it should only contain README.txt while you don't have a stable version.
  2. minor issue in the info file: description=Project management tool should be description = Project management tool. Please see http://drupal.org/node/171205.
  3. line 42 of iprojectweb.module has this line: $map = $_REQUEST;. $_REQUEST can contain user supplied data and thus $map (or rather its values) should check_plain'ed or whatever is appropriate for that context.
  4. line 48 of iprojectweb.module has this line: DEFINE('_IPROJECTWEB_APPLICATION_ROOT', $base); As far I can see you only use this define once and thus I think it could be omitted completely. This is more my taste than an actual issue.
  5. line 49 of iprojectweb.module has this line: DEFINE('DR_DS', DIRECTORY_SEPARATOR); I found this abbreviation to be rather confusing. I know using DIRECTORY_SEPARATOR requires but it also more readable to other developers and if you look at your code after a while you won't DR_DR means either.
  6. could it be that js/as.js is third-party code? If so, you might be asked to use the Libraries API instead.
  7. The file styles/std/iprojectweb_getstyle.inc should be in the module root instead.
  8. I have only looked at your info file, module file and a few other files. There are way too many review them all. Maybe you could find a way to combine a few of them? Of course only if it's possible at all.
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.