Index: openacs-4/packages/workflow/tcl/action-procs.tcl
===================================================================
RCS file: /usr/local/cvsroot/openacs-4/packages/workflow/tcl/action-procs.tcl,v
diff -u -N -r1.12 -r1.13
--- openacs-4/packages/workflow/tcl/action-procs.tcl	1 Dec 2003 09:53:19 -0000	1.12
+++ openacs-4/packages/workflow/tcl/action-procs.tcl	9 Dec 2003 15:46:43 -0000	1.13
@@ -23,7 +23,7 @@
     {-workflow_id:required}
     {-action_id {}}
     {-sort_order {}}
-    {-short_name:required}
+    {-short_name {}}
     {-pretty_name:required}
     {-pretty_past_tense {}}
     {-edit_fields {}}
@@ -108,6 +108,11 @@
                 -sort_order $sort_order
         }
 
+        set short_name [workflow::action::generate_short_name \
+                            -workflow_id $workflow_id \
+                            -pretty_name $pretty_name \
+                            -short_name $short_name]
+
         if { [empty_string_p $action_id] } {
             set action_id [db_nextval "workflow_actions_seq"]
         }
@@ -145,7 +150,9 @@
     }
 
     if { !$internal_p } {
-        workflow::action::flush_cache -workflow_id $workflow_id
+        # Flush the workflow cache, as changing an action changes the entire workflow
+        # e.g. initial_action_p, enabled_in_states.
+        workflow::flush_cache -workflow_id $workflow_id
     }
     
     return $action_id
@@ -174,18 +181,21 @@
     @see workflow::action::new
 } {
     upvar 1 $array row
+    if { ![array exists row] } {
+        error "Array $array does not exist or is not an array"
+    }
     foreach name [array names row] {
         set missing_elm($name) 1
     }
 
-    set set_clauses [list]
-
     if { [empty_string_p $workflow_id] } {
         set workflow_id [workflow::action::get_element \
                              -action_id $action_id \
                              -element workflow_id]
     }
 
+    set set_clauses [list]
+
     # Handle columns in the workflow_actions table
     foreach attr { 
         short_name pretty_name pretty_past_tense edit_fields description description_mime_type sort_order
@@ -197,6 +207,20 @@
             set varname attr_$attr
             # Convert the Tcl value to something we can use in the query
             switch $attr {
+                short_name {
+                    if { ![exists_and_not_null row(pretty_name)] } {
+                        if { [empty_string_p $row(short_name)] } {
+                            error "You cannot edit with an empty short_name without also setting pretty_name"
+                        } else {
+                            set row(pretty_name) {}
+                        }
+                    }
+                        
+                    set $varname [workflow::action::generate_short_name \
+                                      -workflow_id $workflow_id \
+                                      -pretty_name $row(pretty_name) \
+                                      -short_name $row(short_name)]
+                }
                 always_enabled_p {
                     set $varname [db_boolean [template::util::is_true $row($attr)]]
                 }
@@ -291,7 +315,7 @@
 
         # Check that there are no unknown attributes
         if { [llength [array names missing_elm]] > 0 } {
-            error "Trying to set illegal action attributes: [join [array names row] ", "]"
+            error "Trying to set illegal action attributes: [join [array names missing_elm] ", "]"
         }
 
         if { !$internal_p } {
@@ -300,7 +324,9 @@
     }
 
     if { !$internal_p } {
-        workflow::action::flush_cache -workflow_id $workflow_id
+        # Flush the workflow cache, as changing an action changes the entire workflow
+        # e.g. initial_action_p, enabled_in_states.
+        workflow::flush_cache -workflow_id $workflow_id
     }
 
     return $action_id
@@ -496,8 +522,67 @@
     }
 }
 
+ad_proc -public workflow::action::get_existing_short_names {
+    {-workflow_id:required}
+    {-ignore_action_id {}}
+} {
+    Returns a list of existing action short_names in this workflow.
+    Useful when you're trying to ensure a short_name is unique, 
+    or construct a new short_name that is guaranteed to be unique.
 
+    @param ignore_action_id   If specified, the short_name for the given action will not be included in the result set.
+} {
+    set result [list]
 
+    foreach action_id [workflow::get_actions -workflow_id $workflow_id] {
+        if { [empty_string_p $ignore_action_id] || ![string equal $ignore_action_id $action_id] } {
+            lappend result [workflow::action::get_element -action_id $action_id -element short_name]
+        }
+    }
+
+    return $result
+}
+
+ad_proc -public workflow::action::generate_short_name {
+    {-workflow_id:required}
+    {-pretty_name:required}
+    {-short_name {}}
+    {-action_id {}}
+} {
+    Generate a unique short_name from pretty_name.
+    
+    @param action_id    If you pass in this, we will allow that action's short_name to be reused.
+    
+} {
+    set existing_short_names [workflow::action::get_existing_short_names \
+                                  -workflow_id $workflow_id \
+                                  -ignore_action_id $action_id]
+    
+    if { [empty_string_p $short_name] } {
+        if { [empty_string_p $pretty_name] } {
+            error "Cannot have empty pretty_name when short_name is empty"
+        }
+        set short_name [util_text_to_url \
+                            -replacement "_" \
+                            -existing_urls $existing_short_names \
+                            -text $pretty_name]
+    } else {
+        # Make lowercase, remove illegal characters
+        set short_name [string tolower $short_name]
+        regsub -all {[- ]} $short_name {_} short_name
+        regsub -all {[^a-zA-Z_0-9]} $short_name {} short_name
+
+        if { [lsearch -exact $existing_short_names $short_name] != -1 } {
+            error "Action with short_name '$short_name' already exists in this workflow."
+        }
+    }
+
+    return $short_name
+}
+
+
+
+
 ######################################################################
 #
 # workflow::action::fsm
@@ -508,7 +593,7 @@
     {-workflow_id:required}
     {-action_id {}}
     {-sort_order {}}
-    {-short_name:required}
+    {-short_name {}}
     {-pretty_name:required}
     {-pretty_past_tense {}}
     {-edit_fields {}}
@@ -517,6 +602,8 @@
     {-privileges {}}
     {-enabled_states {}}
     {-assigned_states {}}
+    {-enabled_state_ids {}}
+    {-assigned_state_ids {}}
     {-new_state {}}
     {-callbacks {}}
     {-always_enabled_p f}
@@ -571,8 +658,11 @@
         db_dml insert_fsm_action {}
 
         array set update_cols [list]
-        set update_cols(enabled_states) $enabled_states
-        set update_cols(assigned_states) $assigned_states
+        foreach col { enabled_states enabled_state_ids assigned_states assigned_state_ids } {
+            if { ![empty_string_p [set $col]] } {
+                set update_cols($col) [set $col]
+            }
+        }
 
         workflow::action::fsm::edit \
             -internal \
@@ -582,9 +672,11 @@
 
         workflow::definition_changed_handler -workflow_id $workflow_id
     }   
+    
+    # Flush the workflow cache, as changing an action changes the entire workflow
+    # e.g. initial_action_p, enabled_in_states.
+    workflow::flush_cache -workflow_id $workflow_id
 
-    workflow::action::flush_cache -workflow_id $workflow_id
-
     return $action_id
 }
 
@@ -611,8 +703,12 @@
     @see workflow::action::fsm::new
 } {
     upvar 1 $array org_row
+    if { ![array exists org_row] } {
+        error "Array $array does not exist or is not an array"
+    }
 
-    # We make a copy here, so the check for illegal attributes in workflow::action::edit works properly
+    # We make a copy here and work on that, so the check for illegal attributes in workflow::action::edit works properly, 
+    # i.e. we delete from 'row' before calling workflow::action::edit, but we don't touch the caller's row
     array set row [array get org_row]
 
     if { [empty_string_p $workflow_id] } {
@@ -647,6 +743,13 @@
                 db_dml insert_enabled_state {}
             }
             unset row(enabled_states)
+        } elseif { [info exists row(enabled_state_ids)] } {
+            set assigned_p "f"
+            db_dml delete_enabled_states {}
+            foreach enabled_state_id $row(enabled_state_ids) {
+                db_dml insert_enabled_state {}
+            }
+            unset row(enabled_state_ids)
         }
 
         # Record where the action is both enabled and assigned
@@ -660,8 +763,15 @@
                 db_dml insert_enabled_state {}
             }
             unset row(assigned_states)
+        } elseif { [info exists row(assigned_state_ids)] } {
+            set assigned_p "t"
+            db_dml delete_enabled_states {}
+            foreach enabled_state_id $row(assigned_state_ids) {
+                db_dml insert_enabled_state {}
+            }
+            unset row(assigned_state_ids)
         }
-
+ 
         # This will error if there are attributes it doesn't know about, so we remove the attributes we know above
         workflow::action::edit \
             -internal \
@@ -675,7 +785,9 @@
     }
 
     if { !$internal_p } {
-        workflow::action::flush_cache -workflow_id $workflow_id
+        # Flush the workflow cache, as changing an action changes the entire workflow
+        # e.g. initial_action_p, enabled_in_states.
+        workflow::flush_cache -workflow_id $workflow_id
     }
 }
 
@@ -882,7 +994,7 @@
     }
 
     # Flush the thread global cache
-    util_memoize_flush [list workflow::action::get_all_info_not_cached -workflow_id $workflow_id]    
+    util_memoize_flush [list workflow::action::get_all_info_not_cached -workflow_id $workflow_id]
 }
 
 ad_proc -private workflow::action::refresh_request_cache { workflow_id } {
@@ -1035,12 +1147,16 @@
 
     # Build arrays of enabled and assigned state short names for all actions
     array set enabled_states {}
+    array set enabled_state_ids {}
     array set assigned_states {}
-    db_foreach action_enabled_short_name {} {
+    array set assigned_state_ids {}
+    db_foreach action_enabled_in_states {} {
         if { [string equal $assigned_p "t"] } {
             lappend assigned_states($action_id) $short_name
+            lappend assigned_state_ids($action_id) $state_id
         } else {
             lappend enabled_states($action_id) $short_name
+            lappend enabled_state_ids($action_id) $state_id
         }
     }
 
@@ -1049,7 +1165,7 @@
     foreach action_id $action_ids {
         array set one_action $action_data($action_id)
 
-        foreach array_name { privileges enabled_states assigned_states callbacks allowed_roles allowed_role_ids } {
+        foreach array_name { privileges enabled_states enabled_state_ids assigned_states assigned_state_ids callbacks allowed_roles allowed_role_ids } {
             if { [info exists ${array_name}($action_id)] } {
                 set one_action(${array_name}) [set ${array_name}($action_id)]
             } else {
Index: openacs-4/packages/workflow/tcl/action-procs.xql
===================================================================
RCS file: /usr/local/cvsroot/openacs-4/packages/workflow/tcl/action-procs.xql,v
diff -u -N -r1.9 -r1.10
--- openacs-4/packages/workflow/tcl/action-procs.xql	18 Nov 2003 17:57:57 -0000	1.9
+++ openacs-4/packages/workflow/tcl/action-procs.xql	9 Dec 2003 15:46:43 -0000	1.10
@@ -89,9 +89,10 @@
     </querytext>
   </fullquery>
 
-  <fullquery name="workflow::action::get_all_info_not_cached.action_enabled_short_name">
+  <fullquery name="workflow::action::get_all_info_not_cached.action_enabled_in_states">
     <querytext>
-        select s.short_name,
+        select s.state_id,
+               s.short_name,
                waeis.action_id,
                waeis.assigned_p
         from   workflow_fsm_action_en_in_st waeis,
Index: openacs-4/packages/workflow/tcl/state-procs.tcl
===================================================================
RCS file: /usr/local/cvsroot/openacs-4/packages/workflow/tcl/state-procs.tcl,v
diff -u -N -r1.5 -r1.6
--- openacs-4/packages/workflow/tcl/state-procs.tcl	28 Aug 2003 09:41:59 -0000	1.5
+++ openacs-4/packages/workflow/tcl/state-procs.tcl	9 Dec 2003 15:46:43 -0000	1.6
@@ -18,21 +18,31 @@
 
 ad_proc -public workflow::state::fsm::new {
     {-workflow_id:required}
-    {-short_name:required}
+    {-internal:boolean}
+    {-short_name {}}
     {-pretty_name:required}
     {-hide_fields {}}
     {-sort_order {}}
 } {
     Creates a new state for a certain FSM (Finite State Machine) workflow.
     
     @param workflow_id The id of the FSM workflow to add the state to
-    @param short_name
+
+    @param short_name             If you leave blank, the short_name will be generated from pretty_name.
+
     @param pretty_name
+
     @param hide_fields            A space-separated list of the names of form fields which should be
                                   hidden when in this state, because they're irrelevant in a certain state.
+
     @param sort_order             The number which this state should be in the sort ordering sequence. 
                                   Leave blank to add state at the end. If you provide a sort_order number
                                   which already exists, existing states are pushed down one number.
+
+    @param internal     Set this flag if you're calling this proc from within the corresponding proc 
+                        for a particular workflow model. Will cause this proc to not flush the cache 
+                        or call workflow::definition_changed_handler, which the caller must then do.
+
     @return ID of new state.
     
     @author Peter Marklund
@@ -42,22 +52,207 @@
         set state_id [db_nextval "workflow_fsm_states_seq"]
         
         if { [empty_string_p $sort_order] } {
-            set sort_order [workflow::default_sort_order -workflow_id $workflow_id -table_name "workflow_fsm_states"]
+            set sort_order [workflow::default_sort_order \
+                                -workflow_id $workflow_id \
+                                -table_name "workflow_fsm_states"]
         } else {
-            set sort_order_taken_p [db_string select_sort_order_p {}]
-            if { $sort_order_taken_p } {
-                db_dml update_sort_order {}
-            }
+            workflow::state::fsm::update_sort_order \
+                -workflow_id $workflow_id \
+                -sort_order $sort_order
         }
+
+        set short_name [workflow::state::fsm::generate_short_name \
+                            -workflow_id $workflow_id \
+                            -pretty_name $pretty_name \
+                            -short_name $short_name]
         
         db_dml do_insert {}
+
+        if { !$internal_p } {
+            workflow::definition_changed_handler -workflow_id $workflow_id
+        }
     }
 
-    workflow::state::flush_cache -workflow_id $workflow_id
+    if { !$internal_p } {
+        workflow::flush_cache -workflow_id $workflow_id
+    }
  
     return $state_id
 }
 
+
+ad_proc -public workflow::state::fsm::edit {
+    {-state_id:required}
+    {-array:required}
+    {-workflow_id {}}
+    {-internal:boolean}
+} {
+    Creates a new state for a certain FSM (Finite State Machine) workflow.
+    
+    @param state_id     The id of the FSM state you wish to edit
+
+    @param workflow_id  Optionally specify the workflow_id. If not specified, we will execute a query to find it.
+    
+    @param array        Name of an array in the caller's namespace with attributes to edit.
+
+    @param internal     Set this flag if you're calling this proc from within the corresponding proc 
+                        for a particular workflow model. Will cause this proc to not flush the cache 
+                        or call workflow::definition_changed_handler, which the caller must then do.
+
+    @return ID of the state.
+    
+    @author Peter Marklund
+} {        
+    upvar 1 $array row
+    if { ![array exists row] } {
+        error "Array $array does not exist or is not an array"
+    }
+    foreach name [array names row] {
+        set missing_elm($name) 1
+    }
+
+    if { [empty_string_p $workflow_id] } {
+        set workflow_id [workflow::state::fsm::get_element \
+                             -state_id $state_id \
+                             -element workflow_id]
+    }
+
+    set set_clauses [list]
+
+    # Handle columns in the workflow_fsm_states table
+    foreach attr { 
+        short_name pretty_name hide_fields sort_order
+    } {
+        if { [info exists row($attr)] } {
+            set varname attr_$attr
+
+            # Convert the Tcl value to something we can use in the query
+            switch $attr {
+                short_name {
+                    if { ![exists_and_not_null row(pretty_name)] } {
+                        if { [empty_string_p $row(short_name)] } {
+                            error "You cannot edit with an empty short_name without also setting pretty_name"
+                        } else {
+                            set row(pretty_name) {}
+                        }
+                    }
+                        
+                    set $varname [workflow::state::fsm::generate_short_name \
+                                      -workflow_id $workflow_id \
+                                      -pretty_name $row(pretty_name) \
+                                      -short_name $row(short_name)]
+                }
+                default {
+                    set $varname $row($attr)
+                }
+            }
+
+            # Add the column to the SET clause
+            lappend set_clauses "$attr = :$varname"
+
+            unset missing_elm($attr)
+        }
+    }
+
+    db_transaction {
+    
+        # Update state
+        if { [llength $set_clauses] > 0 } {
+            db_dml update_action "
+                update workflow_fsm_states
+                set    [join $set_clauses ", "]
+                where  state_id = :state_id
+            "
+        }
+
+        # Check that there are no unknown attributes
+        if { [llength [array names missing_elm]] > 0 } {
+            error "Trying to set illegal state attributes: [join [array names missing_elm] ", "]"
+        }
+
+        if { !$internal_p } {
+            workflow::definition_changed_handler -workflow_id $workflow_id
+        }
+    }
+
+    if { !$internal_p } {
+        workflow::flush_cache -workflow_id $workflow_id
+    }
+
+    return $state_id
+}
+
+ad_proc -private workflow::state::fsm::update_sort_order {
+    {-workflow_id:required}
+    {-sort_order:required}
+} {
+    Increase the sort_order of other states, if the new sort_order is already taken.
+} { 
+    set sort_order_taken_p [db_string select_sort_order_p {}]
+    if { $sort_order_taken_p } {
+        db_dml update_sort_order {}
+    }
+}
+
+ad_proc -public workflow::state::fsm::get_existing_short_names {
+    {-workflow_id:required}
+    {-ignore_state_id {}}
+} {
+    Returns a list of existing state short_names in this workflow.
+    Useful when you're trying to ensure a short_name is unique, 
+    or construct a new short_name that is guaranteed to be unique.
+
+    @param ignore_state_id   If specified, the short_name for the given state will not be included in the result set.
+} {
+    set result [list]
+
+    foreach state_id [workflow::fsm::get_states -workflow_id $workflow_id] {
+        if { [empty_string_p $ignore_state_id] || ![string equal $ignore_state_id $state_id] } {
+            lappend result [workflow::state::fsm::get_element -state_id $state_id -element short_name]
+        }
+    }
+
+    return $result
+}
+
+ad_proc -public workflow::state::fsm::generate_short_name {
+    {-workflow_id:required}
+    {-pretty_name:required}
+    {-short_name {}}
+    {-state_id {}}
+} {
+    Generate a unique short_name from pretty_name.
+    
+    @param state_id    If you pass in this, we will allow that state's short_name to be reused.
+    
+} {
+    set existing_short_names [workflow::state::fsm::get_existing_short_names \
+                                  -workflow_id $workflow_id \
+                                  -ignore_state_id $state_id]
+    
+    if { [empty_string_p $short_name] } {
+        if { [empty_string_p $pretty_name] } {
+            error "Cannot have empty pretty_name when short_name is empty"
+        }
+        set short_name [util_text_to_url \
+                            -replacement "_" \
+                            -existing_urls $existing_short_names \
+                            -text $pretty_name]
+    } else {
+        # Make lowercase, remove illegal characters
+        set short_name [string tolower $short_name]
+        regsub -all {[- ]} $short_name {_} short_name
+        regsub -all {[^a-zA-Z_0-9]} $short_name {} short_name
+
+        if { [lsearch -exact $existing_short_names $short_name] != -1 } {
+            error "State with short_name '$short_name' already exists in this workflow."
+        }
+    }
+
+    return $short_name
+}
+
+
 ad_proc -public workflow::state::fsm::get {
     {-state_id:required}
     {-array:required}
Index: openacs-4/packages/workflow/tcl/state-procs.xql
===================================================================
RCS file: /usr/local/cvsroot/openacs-4/packages/workflow/tcl/state-procs.xql,v
diff -u -N -r1.4 -r1.5
--- openacs-4/packages/workflow/tcl/state-procs.xql	5 Mar 2003 17:18:10 -0000	1.4
+++ openacs-4/packages/workflow/tcl/state-procs.xql	9 Dec 2003 15:46:43 -0000	1.5
@@ -1,7 +1,7 @@
 <?xml version="1.0"?>
 <queryset>
 
-  <fullquery name="workflow::state::fsm::new.select_sort_order_p">
+  <fullquery name="workflow::state::fsm::update_sort_order.select_sort_order_p">
     <querytext>
         select count(*)
         from   workflow_fsm_states
@@ -10,7 +10,7 @@
     </querytext>
   </fullquery>
 
-  <fullquery name="workflow::state::fsm::new.update_sort_order">
+  <fullquery name="workflow::state::fsm::update_sort_order.update_sort_order">
     <querytext>
         update workflow_fsm_states
         set    sort_order = sort_order + 1
@@ -38,7 +38,8 @@
 
   <fullquery name="workflow::state::fsm::get_all_info_not_cached.select_states">
     <querytext>
-      select state_id,
+      select workflow_id,
+             state_id,
              sort_order,
              short_name,
              pretty_name,