Merge lp:~zeitgeist/zeitgeist/bug580364 into lp:zeitgeist/0.1

[Seif Lotfy]

I fixed bug 580364
now we support searching for storage templates

[Markus Korn (thekorn)]

Sorry, big NACK from me.
It makes no sense to allow this kind of queries right now, as we have no ways to set the storage state implemented. This is why we decided to throw an exception.

review: Disapprove

[Mikkel Kamstrup Erlandsen (kamstrup)]

Well, I disagree with Markus :-) I think it makes perfect sense to include this - otherwise we have a chicken-and-egg problem.

In theory anyone could set the storage field upon item insertion. It just so happens that the DS we ship is not feature complete. On top of that, 3rd parties could provide their own storage monitor extension which update the storage table correctly (at the very least a networkmanager/connman extension is easy).

Regarding the code:

 1) It looks like testFindStorageNotExistant() is there twice?

 2) Docstrings in the unit tests are not right

 3) Apart from these small things it looks good to me

review: Needs Fixing

[Markus Korn (thekorn)]

Ok, I cave in, fix the things mentioned by Mikkel, run the testsuite and if everything goes well merge it into lp:zeitgeist (but please don't *bzr pull*)

review: Needs Fixing

[Seif Lotfy (seif)]

ok done :)

[lp:~zeitgeist/zeitgeist/bug580364]

1562. By Seif Lotfy on 2010-08-31

fixed docstrings and removed duplicate test

=== modified file '_zeitgeist/engine/main.py'
--- _zeitgeist/engine/main.py	2010-08-02 10:13:12 +0000
+++ _zeitgeist/engine/main.py	2010-08-31 20:50:54 +0000
@@ -220,12 +220,6 @@
 				subject_templates = [Subject(data) for data in template[1]]
 			else:
 				subject_templates = None
-			# first of all we need to check if the query is supported at all
-			# we do not support searching by storage field for now
-			# see LP: #580364
-			if subject_templates is not None:
-				if any(data[Subject.Storage] for data in subject_templates):
-					raise ValueError("zeitgeist does not support searching by 'storage' field")
 			
 			subwhere = WhereClause(WhereClause.AND)
 			
@@ -292,6 +286,10 @@
 							if value:
 								value, negation, wildcard = parse_operators(Subject, getattr(Subject, key.title()), value)
 								subwhere.add_text_condition("subj_%s" %key, value, wildcard, negation)
+						
+						if subject_template.storage:
+							subwhere.add_text_condition("subj_storage", subject_template.storage)
+						
 			except KeyError, e:
 				# Value not in DB
 				log.debug("Unknown entity in query: %s" % e)

=== modified file 'test/engine-test.py'
--- test/engine-test.py	2010-07-28 20:28:48 +0000
+++ test/engine-test.py	2010-08-31 20:50:54 +0000
@@ -866,20 +866,27 @@
 		)
 		self.assertEquals(3, len(ids))
 		
-	def testBug580364(self):
-		""" for now we raise a ValueError if someone wants to search
-		by the storage field, this might change later on. (LP: #580364)"""
-		events = [
-			Event.new_for_values(timestamp=1000, subject_storage="sometext"),
-			Event.new_for_values(timestamp=2000, subject_storage="anotherplace")
-		]
-		ids_in = self.engine.insert_events(events)
-		template = Event.new_for_values(subject_storage="xxxx")
-		
-		self.assertRaises(ValueError, self.engine.find_eventids,
-			TimeRange.always(), [template], StorageState.Any, 10,
-			ResultType.MostRecentEvents
-		)
+	def testFindStorageNotExistant(self):
+		events = [
+			Event.new_for_values(timestamp=1000, subject_storage="sometext"),
+			Event.new_for_values(timestamp=2000, subject_storage="anotherplace")
+		]
+		ids_in = self.engine.insert_events(events)
+		template = Event.new_for_values(subject_storage="xxx")
+		results = self.engine.find_eventids(TimeRange.always(), [template], 
+						StorageState.Any, 10, ResultType.MostRecentEvents)
+		self.assertEquals(0, len(results))
+				
+	def testFindStorage(self):
+		events = [
+			Event.new_for_values(timestamp=1000, subject_storage="sometext"),
+			Event.new_for_values(timestamp=2000, subject_storage="anotherplace")
+		]
+		ids_in = self.engine.insert_events(events)
+		template = Event.new_for_values(subject_storage="sometext")
+		results = self.engine.find_eventids(TimeRange.always(), [template], 
+						StorageState.Any, 10, ResultType.MostRecentEvents)
+		self.assertEquals(1, len(results))
 		
 	def testWildcard(self):
 		import_events("test/data/five_events.js", self.engine)