Forum OpenACS Improvement Proposals (TIPs): TIP #81 (Implemented): User Merge account support

The objective of this TIP is to give oacs the ability to merge user accounts. Another goal is to "free up" the username taken by the duplicated account.

This feature will be useful to fix the damage done when the same person registers twice with two different email addresses.

We wrote a 'very draft' and overview document if you want to see it: https://openacs.org/storage/view/proposals/user-account-merge-um/index_draft_version.html

We've developed this feature and it's been working very good for 3 and half weeks. It is stable and simple.

I commited it on oacs-5-2 but Dave told me that the correct procedure was to propose a TIP because It's a new feature and I made two database changes (Should We keep it on branch oacs-5-2? or remove it from there and add it to HEAD? ) :

1. Altered a table constraint adding new member_state ('merged')

2. Added new plsql function to support this feature in the API of membership rels.

We developed that with these scenarios:

1.- Merge the user items (forums, classes rels, faqs, evaluation items, etc...):
Here we used the new, simple & excellent callbacks feature (https://openacs.org/forums/message-view?message%5fid=274647).

We wrote an implementation per each package and if you want that "your new package" supports it, you should write a new implementation to support the account merge.

2.- Merge the users (emails, info, etc)
We have the from_user_id and the to_user_id, the from_user_id account and all their items will be added to the to_user_id account.

We added the operation "MergeUser" to the contract "auth_authentication" and added the implementation for local and ldap authentication. Basically, this part only sets the member_state in 'merged' state and changes the username of the merged account to 'merged_$from_user_id_$to_user_id', just to "free up" that username.

We posted it before here : https://openacs.org/forums/message-view?message%5fid=283291

Collapse
Posted by Enrique Catalan on
This must be for general purpose in oacs (OpenACS , .Lrn, etc), So, I would like to add more details of the Implementation:
  1. We will use two callbacks :
    • MergeShowUserInfo:

      It will return a list of lists with string messages that will say the items to be merged. It will be used in the GUI, so you will be able to see the items of each account before the merge.

    • MergePackageUser:

      This callback is to move the items from the from_user_id to the to_user_id account. For example, .Lrn as a package will have its own implementation that will merge the class memberships and other stuffs. The same for calendar, forums, news, faqs, etc... .

  2. In the other hand, we have the basic user info merge. Here we will follow the authentication approach using the contract "auth_authentication" and adding a new operation named "MergeUser". The implementations of this operation will merge the local and ldap authentication section.
Collapse
Posted by Matthew Geddert on
I haven't given much thought to the way in which this should be done so I won't comment on that. But I wholeheartedly encourage us to figure out a way to merge users. This will be especially useful (if not essential) for the new contacts package which is in CVS HEAD when conbining information from multiple private contact lists, or merging private contacts back into the global list.
Collapse
Posted by Jade Rubick on
I approve, for oacs-5-2 even. New functionality should not go into the current release branch, but since it's already there, I'd rather have Enrique spend the time fixing bugs that going back and committing to a different branch :)
Collapse
Posted by Dave Bauer on
Can you post a ad_proc -callback definition for the proposed callbacks?

ShowUserInfo will return a list of objects "owned" by a package by a user that will be handled by that package in the merge operation?

And will MergePackageUser then update the appropriate data in each package to update the user_ids for the merged user?

Collapse
Posted by Enrique Catalan on
Of course. The ad_proc -callback definitions should be:
ad_proc -callback MergeShowUserInfo {
    -user_id:required
} {
    Show the items that user_id owns

} {}

ad_proc -callback MergePackageUser {
    -from_user_id:required
    -to_user_id:required
} {
    Merge two accounts
} {}

>>ShowUserInfo will return a list of objects "owned" by a package by a user that will be handled by that package in the merge operation?

Yes, each implementation returns a list of items of its package owned or related with the user "to merge".

>>And will MergePackageUser then update the appropriate data in each package to update the user_ids for the merged user?

Yes, it'll do that.

Collapse
Posted by Andrew Grumet on
Approve -- let's make this work.

I took a brief look at the oacs-5-2 code. It's buggy on Oracle, for example there are some calls to acs_object__name (postgres-specific) in merge.xql.

Also, I don't understand this code in merge-final.tcl

set user_res [acs_sc::invoke \
-error \
-contract "auth_authentication" \
-impl_id $impl_id \
-operation MergeUser \
-call_args $parameters]

When I look at the service contract page for auth_authentication under /acs-service/contract, I don't see a MergeUser operation. Perhaps this is old code that should be removed?

Collapse
Posted by Don Baccus on
The Oracle version broke the install of acs-kernel ... fixed that yesterday.

Andrew, can you fix the simple query error you've found?

We need to get the service contract issue resolved, too.

This is confirming my lack of enthusiasm for adding features to a version which has already been feature-freezed just because "it is needed" and "it works".

It may be simple, but it is clearly not stable for Oracle ...

Collapse
Posted by Enrique Catalan on
You will not find the implementations of the callback yet and the implementations of the service contract neither, I stopped 'the commit' of this code when I received the email of Dave, We've been waiting your answers since we posted this to continue contributing this feature :) , so We just need to know in wich branch we could commit it.

Thanks for fix the bug Don and Andrew. About the service contract operation, could we commit that code into oacs52?

Collapse
Posted by Andrew Grumet on
Hmm, modifying an existing service contract could get messy or others, even if it seems to be working great for you. They may for example be working with SC implementations we don't know about. If this is required, I'd stay off oacs-5-2.
Collapse
Posted by Don Baccus on
Also we'd need upgrade scripts not just install scripts for both oracle and pg, TESTED for both systems, Quio ...
Collapse
Posted by Malte Sussdorff on
In general the approach should be to put it into HEAD (after the TIP has been approved) and then, once it works on both Oracle and PostgreSQL and works with the upgrades you can do a backport. But I'm not sure how other OCT Members feel about it.
Collapse
Posted by Enrique Catalan on
Andrew: the new SC operation is required because we need to manage the merge operation in authentication. By now, we have two implementations; One for local and the other one for auth-ldap.

This is the part of the merge process where we have to be careful. This time i'm posting some tcl code to get it clearly:

Adding the new operation :

ad_proc -private auth::local::authentication::register_impl {} {
    Register the 'local' implementation of the 'auth_authentication' service contract.

    @return impl_id of the newly created implementation.
} {
    set spec {
        contract_name "auth_authentication"
        owner "acs-authentication"
        name "local"
        pretty_name "Local"
        aliases {
            MergeUser auth::local::authentication::MergeUser
            Authenticate auth::local::authentication::Authenticate
            GetParameters auth::local::authentication::GetParameters
        }
    }

    return [acs_sc::impl::new_from_spec -spec $spec]
}

One implementation is:

ad_proc -private auth::local::authentication::MergeUser {
    from_user_id
    to_user_id
    {authority_id ""}
} {
    Merge operation of the auth_authentication
    Here we will merge the basic user info
    and merge the email and all related with the username
} {
    ns_log Notice "Starting local merge user"
    db_transaction {
        ns_log Notice "Merging user portrait"
        if { ( ![db_0or1row to_user_portrait { *SQL*} ] ) &&  ( [db_0or1row from_user_portrait { *SQL* } ] )  } {
            db_dml upd_portrait { *SQL* }
            ns_log Notice "Merging user portrait"
        }

        # get the permissions of the from_user_id
        # and grant them to the to_user_id
        db_foreach getfromobjs { *SQL* } {
            # revoke the permissions from from_user_id
            permission::revoke -object_id $from_oid -party_id $from_user_id -privilege $from_priv
            if { ![db_string getdata { *SQL* } ] } {
                # grant the permissions to to_user_id
                permission::grant -object_id $from_oid -party_id $to_user_id -privilege $from_priv
            }
        }

        lappend res "acs_permissions merged"

        ns_log notice "   Merging acs_objects ..."

        db_dml acs_objs_upd  { *SQL* }

        set msg "acs_objects merged"
        ns_log notice $msg
        lappend res $msg

        ns_log notice "   Merging user,names and email..."
        ns_log Notice "     Deleting user $from_user_id. It will be merged with user_id $to_user_id"

        set new_username "merged_$from_user_id"
        append new_username "_$to_user_id"

        # Shall we keep the domain for email?
        # Actually, the username 'merged_xxx_yyy'
        # won't be an email, so we will keep it without
        # domain
        set new_email "$new_username"

        set rel_id [db_string getrelid { *SQL* }]
        membership_rel::change_state -rel_id $rel_id -state "merged"
        ns_log Notice "     state changed, rel_id $rel_id, from_user_id ; $from_user_id"

        acs_user::update -user_id $from_user_id -username "$new_username" -screen_name "$new_username"
        party::update -party_id $from_user_id -email "$new_email"

        set msg "     username and member state done"
        ns_log notice $msg
        lappend res $msg
    }
    lappend res "local MergeUser is complete"
    ns_log notice $res
    return $res
}

The script for upgrade is in .../acs-authentication/tcl/apm-callback-procs.tcl, the code is in auth::after_upgrade callback and it is:

.
..
...
        5.1.4 5.1.5 {
                db_transaction {
                    # Following the above steps to upgrade a SC
                    # I will add support to MergeUser operation (quio)
                    ns_log notice "Starting Upgrade"
                    acs_sc::contract::operation::new \
                        -contract_name "auth_authentication" \
                        -operation "MergeUser" \
                        -input { from_user_id:integer to_user_id:integer authority_id:integer } \
                        -output {} \
                        -description "Merges two accounts given the user_id of each one"
                    acs_sc::impl::alias::new \
                        -contract_name "auth_authentication" \
                        -impl_name "LDAP" \
                        -operation "MergeUser" \
                        -alias "auth::ldap::authentication::MergeUser" \
                    acs_sc::impl::alias::new \
                        -contract_name "auth_authentication" \
                        -impl_name "local" \
                        -operation "MergeUser" \
                        -alias "auth::local::authentication::MergeUser" \
                    ns_log notice "Finishing upgrade"

                }
            }
Collapse
Posted by Enrique Catalan on
Don:

Actually, those sql acs-kernel scripts exist. The pg script has been tested and it is:

-- procedure merge
create or replace function membership_rel__merge (integer)
returns integer as '
declare
  merge__rel_id                alias for $1;
begin
    update membership_rels
    set member_state = ''merged''
    where rel_id = merge__rel_id;

    return 0;
end;' language 'plpgsql';

alter table membership_rels drop constraint membership_rel_mem_ck;

alter table membership_rels add constraint membership_rel_mem_ck check (member_state in ('approved','needs approval','banned','rejected','deleted','merged'));

In Oracle, I just alter the table constraint and recreate the definition and body of the package 'membership_rel' and it is :

-- Add support for merge member state

alter table membership_rels drop constraint membership_rel_mem_ck;

alter table membership_rels add constraint membership_rel_mem_ck check (member_state in ('approved','needs approval','banned','rejected','deleted','merged'));

create or replace package membership_rel as function new ( rel_id in membership_rels.rel_id%TYPE default null, rel_type in acs_rels.rel_type%TYPE default 'membership_rel', object_id_one in acs_rels.object_id_one%TYPE, object_id_two in acs_rels.object_id_two%TYPE, member_state in membership_rels.member_state%TYPE default 'approved', creation_user in acs_objects.creation_user%TYPE default null, creation_ip in acs_objects.creation_ip%TYPE default null ) return membership_rels.rel_id%TYPE; procedure ban ( rel_id in membership_rels.rel_id%TYPE ); procedure approve ( rel_id in membership_rels.rel_id%TYPE ); procedure merge ( rel_id in membership_rels.rel_id%TYPE ); procedure reject ( rel_id in membership_rels.rel_id%TYPE ); procedure unapprove ( rel_id in membership_rels.rel_id%TYPE ); procedure deleted ( rel_id in membership_rels.rel_id%TYPE ); procedure del ( rel_id in membership_rels.rel_id%TYPE ); function check_representation ( rel_id in membership_rels.rel_id%TYPE ) return char;

end membership_rel; / show errors

create or replace package body membership_rel as function new ( rel_id in membership_rels.rel_id%TYPE default null, rel_type in acs_rels.rel_type%TYPE default 'membership_rel', object_id_one in acs_rels.object_id_one%TYPE, object_id_two in acs_rels.object_id_two%TYPE, member_state in membership_rels.member_state%TYPE default 'approved', creation_user in acs_objects.creation_user%TYPE default null, creation_ip in acs_objects.creation_ip%TYPE default null ) return membership_rels.rel_id%TYPE is v_rel_id integer; begin v_rel_id := acs_rel.new ( rel_id => rel_id, rel_type => rel_type, object_id_one => object_id_one, object_id_two => object_id_two, context_id => object_id_one, creation_user => creation_user, creation_ip => creation_ip ); insert into membership_rels (rel_id, member_state) values (v_rel_id, new.member_state); return v_rel_id; end; procedure ban ( rel_id in membership_rels.rel_id%TYPE ) is begin update membership_rels set member_state = 'banned' where rel_id = ban.rel_id; end; procedure approve ( rel_id in membership_rels.rel_id%TYPE ) is begin update membership_rels set member_state = 'approved' where rel_id = approve.rel_id; end; procedure merge ( rel_id in membership_rels.rel_id%TYPE ) is begin update membership_rels set member_state = 'merged' where rel_id = approve.rel_id; end; procedure reject ( rel_id in membership_rels.rel_id%TYPE ) is begin update membership_rels set member_state = 'rejected' where rel_id = reject.rel_id; end; procedure unapprove ( rel_id in membership_rels.rel_id%TYPE ) is begin update membership_rels set member_state = 'needs approval' where rel_id = unapprove.rel_id; end; procedure deleted ( rel_id in membership_rels.rel_id%TYPE ) is begin update membership_rels set member_state = 'deleted' where rel_id = deleted.rel_id; end; procedure del ( rel_id in membership_rels.rel_id%TYPE ) is begin acs_rel.del(rel_id); end; function check_index ( group_id in groups.group_id%TYPE, member_id in parties.party_id%TYPE, container_id in groups.group_id%TYPE ) return char is result char(1); n_rows integer; begin select count(*) into n_rows from group_member_index where group_id = check_index.group_id and member_id = check_index.member_id and container_id = check_index.container_id; if n_rows = 0 then result := 'f'; acs_log.error('membership_rel.check_representation', 'Row missing from group_member_index: ' || 'group_id = ' || group_id || ', ' || 'member_id = ' || member_id || ', ' || 'container_id = ' || container_id || '.'); end if; for row in (select r.object_id_one as container_id from acs_rels r, composition_rels c where r.rel_id = c.rel_id and r.object_id_two = group_id) loop if check_index(row.container_id, member_id, container_id) = 'f' then result := 'f'; end if; end loop; return result; end; function check_representation ( rel_id in membership_rels.rel_id%TYPE ) return char is group_id groups.group_id%TYPE; member_id parties.party_id%TYPE; result char(1); begin result := 't'; if acs_object.check_representation(rel_id) = 'f' then result := 'f'; end if; select r.object_id_one, r.object_id_two into group_id, member_id from acs_rels r, membership_rels m where r.rel_id = m.rel_id and m.rel_id = check_representation.rel_id; if check_index(group_id, member_id, group_id) = 'f' then result := 'f'; end if; for row in (select * from group_member_index where rel_id = check_representation.rel_id) loop if composition_rel.check_path_exists_p(row.container_id, row.group_id) = 'f' then result := 'f'; acs_log.error('membership_rel.check_representation', 'Extra row in group_member_index: ' || 'group_id = ' || row.group_id || ', ' || 'member_id = ' || row.member_id || ', ' || 'container_id = ' || row.container_id || '.'); end if; end loop; return result; end;

end membership_rel; / show errors

I'll test it now, what do you think?

Collapse
Posted by Rocael Hernández Rizzardini on
approved, Enrique will test upgrades for oracle & pg.
Collapse
Posted by Enrique Catalan on
I've implemented your advices and modifications to the core of user merge feature and now it's working on oacs-5-2. This core includes modifications in acs-tcl, acs-kernel, acs-authentication and the GUI (acs-admin). It is commited and tested for pg and oracle.

I've tested the sql install and upgrade scripts too (oracle and pg). They are 'drive&run' ;).

In Addition of the core of this stuff, I'll commit, as soon as possible, the 'MergePackageUser' and 'MergeShowUserInfo' implementations of most of the packages.

Collapse
Posted by Andrew Grumet on
I've done some basic testing of this on Oracle on a fresh install.

The UI looks great!

I've committed a small change to merge.tcl, which was erroring out of the users created content in the system.

The biggest issue I ran into so far is, I noticed that the object reassignments are handled in /packages/acs-authentication/tcl/local-procs.tcl. That doesn't seem right. The object reassignments must be handled regardless of what authentication is being used. I think this code should be moved, either directly into /packages/acs-admin/www/users/merge-final.* or to a library in /packages/acs-admin/tcl/.

Collapse
Posted by Enrique Catalan on
Yes Andrew, It didn't seem right. I moved this code to the acs-admin/tcl/merge-procs.tcl library as you said. I moved the 'merge portrait' code and the 'permissions merge' too.

It's already committed.

Collapse
Posted by Enrique Catalan on
Ok, here we go with the implementations.

I commited some packages implementations for this. In the case of Faq, File Storage, CR and basic information of other packages that are based on ACS Objects, the merge is easy because we just need to update the acs_objects table and check the permissions (these stuffs are done by um core).

The other case is when we have to update 'specific data' of the package, for example : Calendar items, .LRN memberships, etc. I've been writing the implementations for Calendar, Notifications, Photo Album, Wimpy Point, Auth LDAP, Forums, News, Lars Blogger, .lrn, lors, evaluation, assessments.

In all of those cases the queries are just a 'select' or 'update' (without pl/sql calls), so I added that in the generic .xql files and this means that it works for ora & pg :).

Collapse
Posted by Dave Bauer on
Approved.

This can be tested on the development copy of OpenACS.org after it is upgraded to 5.2.

Collapse
21: Re: TIP #81: approve (response to 1)
Posted by Don Baccus on
Need the feature, looks like it's mostly been committed anyway :)
Collapse
Posted by Caroline Meeks on
Approved
Collapse
Posted by Andrew Grumet on
Approve.