Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1245 (closed defect: fixed)

Opened 11 years ago

Last modified 10 years ago

identity logout does not work with set_identity_user (SQLObject)

Reported by: Felix.Schwarz Owned by: alberto
Priority: normal Milestone: 1.0.1
Component: Identity Version: 1.0
Severity: normal Keywords:
Cc:

Description

I'm using the new possibilities from ticket #1166 in order to test my applications. Unfortunately, logout does not work correctly because there is no visit key. This problem is fixed by the following three patches. I split them up to show the development process and only the first one is really needed to fix this bug. The other ones just do some cleanup.

Attachments

logout.patch Download (2.0 KB) - added by Felix.Schwarz 11 years ago.
This adds a test case for the defect and removes the condition that there must be a visit_key. The resulting SQLObject exception is catched.
avoiding_some_exceptions.patch Download (903 bytes) - added by Felix.Schwarz 11 years ago.
We don't actually *have* to see the exception if we know before the visit_key is None. This may be considered a premature optimization because the there is no test case which uncovers a bug.
cleanup.patch Download (1.5 KB) - added by Felix.Schwarz 11 years ago.
The method looks a bit ugly, this patch does some cleanup. It removes the outer try/catch which was useless anyway since revision 2243 (ticket # 1211).
unified.patch Download (2.4 KB) - added by Felix.Schwarz 11 years ago.
new unified patch

Change History

Changed 11 years ago by Felix.Schwarz

This adds a test case for the defect and removes the condition that there must be a visit_key. The resulting SQLObject exception is catched.

Changed 11 years ago by Felix.Schwarz

We don't actually *have* to see the exception if we know before the visit_key is None. This may be considered a premature optimization because the there is no test case which uncovers a bug.

Changed 11 years ago by Felix.Schwarz

The method looks a bit ugly, this patch does some cleanup. It removes the outer try/catch which was useless anyway since revision 2243 (ticket # 1211).

comment:1 Changed 11 years ago by Felix.Schwarz

  • Summary changed from identity logout does not work with set_identity_user (SQLObject) to [PATCH] identity logout does not work with set_identity_user (SQLObject)

After the refactoring, I think the reason for the original try/catch block was to catch SQLObject exceptions if the visit key was not found in the database (may it was cleared between request arrival and logout). However, I'm not sure if catching SQLObjectNotFound is 100% correct. Is there any possibility that SQLObject throws this exception in another situation such as database gone etc?

comment:2 Changed 11 years ago by alberto

  • Owner changed from anonymous to alberto
  • Status changed from new to assigned

comment:3 Changed 11 years ago by alberto

  • Summary changed from [PATCH] identity logout does not work with set_identity_user (SQLObject) to identity logout does not work with set_identity_user (SQLObject)

Felix, could you please consolidate those 3 patches into a single patch. I'm not sure what you want me to do with them. It seems the last 2 improve the first one, if this is the case they're better in a single patch.

Thanks,

Alberto

Changed 11 years ago by Felix.Schwarz

new unified patch

comment:4 Changed 11 years ago by Felix.Schwarz

I'm not sure what you want me to do with them. It seems the last 2 improve the first one, if this is the case they're better in a single patch.

The reason for the three patches was that the later two were not technically necessary and other projects welcome small incremental patches so that the maintainer can decide which patches to commit and which he does not like. Unified diff is attached now.

comment:5 Changed 11 years ago by alberto

  • Status changed from assigned to closed
  • Resolution set to fixed

Comitted at [2421]. Thanks :)

Alberto

Note: See TracTickets for help on using tickets.