Ticket #1173 (new enhancement)

Opened 7 years ago

Last modified 9 months ago

Refactor user module

Reported by: Tamme Assigned to: Tamme
Priority: normal Milestone:
Component: module - user Severity: in progress
Keywords: Cc:

Description

There are some issues that should handled...

  • DA_User needs consistent transaction control
  • Remove data access in manager functions and move it into DA_User
  • Write unit tests for public DA_User methods

Attachments

ref_user_alpha.diff (45.1 kB) - added by Tamme on 08/22/06 11:02:46.

Change History

08/22/06 11:02:12 changed by Tamme

  • severity changed from open to need feedback.

This is a first alpha version of my work. To keep Seagull running and other modules unaffected I created a new DA_User2 to work with. With this patch only UserMgr and RegisterMgr rely on the new DA_User2. The patch is built against latest svn bugfix branch.

DA_UserTemplate

  • Public API
  • Implements the Template Method Pattern

DA_User2

  • Transactions are handled in the template, DA_User2 contains only "primitive methods"
  • Consistent error handling and returning

UserMgr

  • Much data access stuff moved to DA_User2
  • Some minor changes

Leftover work

  • Public methods will do extensive parameter checking
  • All other managers in user module
  • Improve "change user status" stuff
  • Remove admins' ability to reset a user password since sending the new password via (plain-text) email is pretty unsecure
  • Move login stuff to DA_User
  • Solution for raw SQL queries in manager methods when using a pager
  • Unit tests for public methods

Questions

  • Please tell me what you (conceptionally) think about the _deleteUserPermissions() method in DA_User2. I did it this way because I dislike dozens of methods like
    • deletePermsByUserId
    • deletePermsByUserModuleId
    • deletePermsByThisAndThat
    • deletePermsByFooButNotBar

You see what I mean? Another reason why I dislike it is that it's often simple code duplication with minor changes, e.g. in a WHERE section

  • Comments on ResultCounter and WhereStack are also greatly appreciated

08/22/06 11:02:46 changed by Tamme

  • attachment ref_user_alpha.diff added.

08/29/06 14:30:41 changed by demian

  • severity changed from need feedback to in progress.
  • milestone changed from 0.6.1 to 0.7.0.

Hi Tamme

Thanks for this patch, have just been looking over the proposed code.

I like most of the ideas you have here, here's what i think works well with some comments:

  • template method pattern: this is a mostly abstract class, i would just rename it to DA_AbstractUser or even better AbstractUserDAO which more clearly communicates its role
  • abstracting transaction stuff is a big improvement
  • each concrete class I think should be done in the standard PEAR way, ie modules/user/AbstractUserDAO.php (DAO is an improvement on DA_*), then modules/user/Container/DbDAO.php, modules/user/Container/LdapDAO.php etc.
  • you haven't suggested it but i wonder if using the observer pattern would make for cleaner code, just attach pref/perm/org related "sub-tasks" as listeners
  • WhereStack? and ResultCounter? are nice, why not add them to SGL_Sql so we can take advantage of namespacing
  • i think your _deleteUserPermissions() method is a good improvement

Here's a few things to watch out for:

  • don't rely on PEAR docs for return values, quite often they are out of date and wrong, use your debugger
  • _deleteUserPermissions($aOptions = null) ... then if (is_null($aOptions) return SGL::raiseError ... does not make sense, a default argument cannot be wrong
  • RC_SUCC no need to truncate constant, and prefix it with SGL_

In addition:

// no logMessage allowed here, // Tamme: WHY???

This is because when session is being initialised, the DA_User call is made and logging is not setup yet.

Overall I think the patch is almost fine to go in to 0.7 trunk (that i need to sort out), why not make the few proposed fixes and double check returns values because i think i used false instead of pear::error for a good reason, then i'll commit it.

03/23/08 08:57:19 changed by demian

  • milestone changed from 0.7.0 to 0.9.0 - polishing.

Milestone 0.7.0 deleted

02/26/13 13:14:37 changed by demian

  • cc deleted.
  • milestone deleted.