Ticket #910 (new defect)

Opened 7 months ago

Last modified 5 months ago

Fix and rework user handling

Reported by: ronald Assigned to:
Priority: critical Milestone:
Component: ambra Version: 0.9-SNAPSHOT
Keywords: Cc:
Blocking: Blocked By:

Description

One the one hand user handling is broken: e.g. if you create a new user they get redirected to the set-profile page, but trying to save the profile results in an error. The problem is that we save a PlosOneUser object in the session, which in turn contains a UserProfile object, but we don't re-attach that object to the OTM session on a subsequent request.

While fixing this we should really clean up and get rid of the PlosOneUser class, add caching for the user-objects, and apply access-controls when displaying the profile (instead of when storing it in the session). The profile should either be eager-fetched, or we must make sure to re-attach the user object to the OTM session when retrieved from the cache; all modifications must make sure to re-attach to the session first.

Dependency Graph

Change History

04/14/08 01:56:01 changed by ronald

  • owner deleted.

04/23/08 18:04:18 changed by amit

  • owner set to pradeep.

04/23/08 18:06:14 changed by pradeep

  • status changed from new to assigned.

04/24/08 13:49:05 changed by pradeep

One way to fix is what is described in the description. And the other is to simply evict() the old and replace with the new. eg. this one line patch fixes the editProfile problem:

--- src/main/java/org/plos/user/service/UserService.java        (revision 5522)
+++ src/main/java/org/plos/user/service/UserService.java        (working copy)
@@ -369,6 +369,7 @@
     }
 
     UserAccount ua = getUserAccount(inUser.getUserId());
+    session.evict(ua.getProfile());
     ua.setProfile(inUser.getUserProfile());
     session.saveOrUpdate(ua);

We can close this bug with this patch and let the replacement of PlosOneUser? be another task then?

04/24/08 15:25:54 changed by pradeep

(In [5525]) Explicit evict needed before attaching a replacement to OTM session. This addresses the editProfile problem (re #910) that prevents folks from creating a new user account.

04/24/08 15:28:30 changed by amit

  • milestone deleted.

r5525 solves the error problem for 0.9. Major cleanup for user handling can be done after 0.9. Moving it out of the milestone.

04/30/08 02:22:34 changed by ronald

  • milestone set to 0.9.0.

Unfortunately the problem is not fixed: every time you log in you are asked to update and save your profile, and the profile does not seem to get saved. Moving back to 0.9 Milestone.

05/03/08 04:29:07 changed by pradeep

(In [5585]) Update user-service to not trip on the changes in the 0.8.3 version of OTM. OTM is now state based. So this means once an object is attached to the session, OTM will keep track of any state changes and save it automatically.

PlosOneUser? was updating the UserProfile? object with no intention of saving it. Therefore everytime we create a PlosOneUser?, we detach the UserProfile? from Otm Session so that OTM will not save it.

The other is related to a change in the default cascade. It used to be all + deleteOrphan. Now it is just 'all' by default. So update the user model to explicitly set the cascade mode.

Addresses #910. So create/update user/profile and login/logout should all work now.

05/03/08 10:31:26 changed by amit

  • milestone deleted.

Moving this out of 0.9 again.

05/06/08 01:00:57 changed by pradeep

(In [5633]) Make 'edit-preferences' work again. Addresses #910.

Turns out there is one more nuance to that 'evict' saga for the UserProfile? object. A manual save needs to be called since the UserPreferences? may have been previously evicted. Also after saving, the UserProfile? object, needs to be evicted again since it is likely to be from the PlosOneUser? object stored in the Http-Session.

There could still be a potential problem with this evict strategy. Associations that reference the evicted object can 're-attach' it. So in short it is not a good idea to muck with a persistent object unless the goal is to change its state in the database. So the PlosOneUser? object needs to be fixed for this reason - or use a different Session object to load it.

05/07/08 18:13:10 changed by russ

  • milestone set to 0.9.0.

this is still not working circa r5654. changes to user profile are not saved back to mulgara.

05/12/08 16:02:06 changed by amit

  • owner changed from pradeep to jkirton.
  • status changed from assigned to new.

05/13/08 12:23:19 changed by russ

  • owner changed from jkirton to amit.

after reverting struts-plosone.xml, i get js errors when i view and when i submit the user preferences form.

on view, according to firebug:

[Exception... "Could not convert JavaScript argument" nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)" location: "JS frame :: http://plosone-branch.plos.org:8080/javascript/dojo/dojo/dojo.js.uncompressed.js :: anonymous :: line 3366" data: no]
[Break on this error] node.addEventListener(name, fp, false);
dojo.js.uncompres... (line 3366)

on submit:

ReferenceError: jumpToElement is not defined message=jumpToElement is not defineddojo.js.uncompres... (line 2684)
ReferenceError: jumpToElement is not defined message=jumpToElement is not definedambra.js.uncompre... (line 6018)

as before, the changes are saved to the user session but not to mulgara. i suspect that javascript is not the problem, but rather the previous commits on this ticket did not actually fix the problem.

05/13/08 12:23:27 changed by russ

  • owner changed from amit to jkirton.

05/15/08 12:27:38 changed by jkirton

  • status changed from new to assigned.

05/16/08 14:56:33 changed by jkirton

(In [5769]) addresses #895, #910 Moved jumpToElement() method to the general.js module for accessibility beyond the article view page. This eliminates a js error that was occurring for form submission to the editPrefsAlerts action.

05/16/08 14:56:36 changed by jkirton

(In [5770]) addresses #895, #910 Moved jumpToElement() method to the general.js module for accessibility beyond the article view page. This eliminates a js error that was occurring for form submission to the editPrefsAlerts action.

05/16/08 15:15:16 changed by jkirton

(In [5777]) addresses #910 Eliminated js related errors NOTE: Currently there are some css/styling issues so the content looks jarbled at the moment. Styling fixes pending..

05/19/08 14:00:30 changed by russ

  • owner changed from jkirton to pradeep.
  • status changed from assigned to new.

no more javascript errors, however, changes are still not preserved between sessions.

(follow-up: ↓ 21 ) 05/28/08 17:18:17 changed by russ

it looks like create new user is working - the user is saved to mulgara.

however, subsequent changes to preferences for existing users are not being saved to mulgara.

(in reply to: ↑ 20 ) 05/30/08 15:26:28 changed by pradeep

  • owner changed from pradeep to russ.

Replying to russ:

it looks like create new user is working - the user is saved to mulgara. however, subsequent changes to preferences for existing users are not being saved to mulgara.

Hmm. I am having a difficult time reproducing this. I have a server running on moody. Could you give it a try on that so that I have the logs to take a look? Thanks.

06/03/08 16:56:40 changed by amit

I just confirmed this on black.topazproject.org. Changing 'city' from San Francisco to Entebbe worked and was saved and retrieved. However changing alerts was saved, but it not being retrieved correctly. It is in the cache. I suspect that this might be a dojo problem. Jon, can you please take a look at this?

Russ, one of the easiest ways to confirm the save is to look at the Mulgara txn log in /var/lib/topaz/... or runitql and retrieve the list of $s $p $o.

06/03/08 16:56:57 changed by amit

  • priority changed from high to critical.

06/03/08 17:09:26 changed by amit

  • owner changed from russ to jkirton.

Thought I had assigned this to Jon.

06/04/08 14:09:10 changed by jkirton

  • status changed from new to assigned.

06/06/08 01:00:32 changed by pradeep

(In [5869]) evict() should remove from proxies list also.

(re #910. Was causing the replaced profile object being treated as a pristine proxy and thus preventing the user profile object from being saved)

06/06/08 11:51:13 changed by jkirton

(In [5875]) Merged user_profile.js and init_profile.js to a file named: user_profile.js since they are only ever referenced in tandem. Cleaned up the js code a bit in the process.

re #910 - Determined that js/xhr is not causing user alerts from being properly re-displayed.

06/10/08 17:54:50 changed by pradeep

(In [5906]) evicted items are re-attached simply by being referenced by an attached object and save is set to cascade. In the case of UserAccount?, we do want the save to cascade. So evict in itself is not sufficient for the UserProfile? object. So now PlosOneUser? keeps a clone() of the UserProfile? object. It is safer, since PlosOneUser? now have no OTM persistable POJOs. Fixes #958. Also see #910.

06/12/08 14:22:18 changed by jkirton

(In [5934]) addresses #910

  • Created new freemarker context property: orgName that derives from the config property: ambra.platform.name.
  • Changed freemarker context property: 'plosOneHost' to 'host' to make generic.

  • Replaced all orgName freemarker variable declarations, as a result of r5931, with the newly created freemarker context property (i.e. freemarker_config.orgName) save for displayUser.ftl which locally defines orgName. This may need clarification..

06/13/08 16:53:22 changed by amit

  • owner changed from jkirton to pradeep.
  • status changed from assigned to new.

Reassigning to Pradeep based on Jon's email to mailing list.

06/16/08 16:16:20 changed by pradeep

  • status changed from new to closed.
  • resolution set to fixed.

(In [5976]) Make the raw/un-loaded field values un-available after a lazy-loaded collection is loaded. This is because the values here could be stale if the application modified the collection contents.

This was causing the Object-Cache in ambra to contain stale entries for the Set<UserPreference?> in UserPreferences?. This resulted in updates to the alerts not be visible. Fixes #910.

06/16/08 16:30:09 changed by pradeep

  • priority changed from critical to low.
  • status changed from closed to reopened.
  • resolution deleted.
  • milestone deleted.

Sorry. Didn't mean to close this ticket, since the original ticket was about replacing PlosOneUser?.

Replacing PlosOneUser? is something that could be considered for a later release. For now there is no persistent components in the PlosOneUser? and hence it is a true 'DTO' object. So ...

06/16/08 16:41:44 changed by amit

  • owner deleted.
  • priority changed from low to high.
  • status changed from reopened to new.

06/25/08 09:08:35 changed by rich

  • priority changed from high to critical.
  • milestone set to 0.9.0.

Still can't save your user preferences in user/secure/editProfile.action. This is with build 6017

06/25/08 09:26:33 changed by rich

  • milestone deleted.

Sorry - should have re-opened 973