Merge lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-lp1009630 into lp:ubuntu-accomplishments-daemon

Proposed by Matt Fischer
Status: Merged
Merged at revision: 111
Proposed branch: lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-lp1009630
Merge into: lp:ubuntu-accomplishments-daemon
Diff against target: 297 lines (+102/-42)
2 files modified
accomplishments/daemon/api.py (+65/-40)
accomplishments/daemon/tests/tests.py (+37/-2)
To merge this branch: bzr merge lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-lp1009630
Reviewer Review Type Date Requested Status
Ubuntu Accomplishments Daemon Developers Pending
Review via email: mp+115786@code.launchpad.net

Description of the change

1) Whitespace clean-up (next time I will fix all of this so I dont have to keep doing it)
2) tests for run_script/run_scripts
3) fixes 1009630 - checks to ensure all required info for a given accomID is there before running the script
4) I rewrote run_scripts() so it's cleaner and easier to follow the flow.

If you look at _is_all_extra_information_available() I'm not sure about how I did the [0] part in the loop at the bottom of the function, does it look right?

Here's the relevant section of log after I run it:

2012-07-19 17:37:19+0100 [-] Adding to scripts queue: ['ubuntu-community/registered-on-launchpad', 'ubuntu-community/registered-on-askubuntu']
2012-07-19 17:37:19+0100 [-] --- Starting Running Scripts - 2 items on the queue ---
2012-07-19 17:37:19+0100 [-] Running ubuntu-community/registered-on-launchpad, left on queue: 1
2012-07-19 17:37:19+0100 [-] launchpad-email is missing for ubuntu-community/registered-on-launchpad, is_all_extra_information_available returning False
2012-07-19 17:37:19+0100 [-] ...Extra information required, but not available, skipping
2012-07-19 17:37:19+0100 [-] Running ubuntu-community/registered-on-askubuntu, left on queue: 0
2012-07-19 17:37:19+0100 [-] askubuntu-user-url is missing for ubuntu-community/registered-on-askubuntu, is_all_extra_information_available returning False
2012-07-19 17:37:19+0100 [-] ...Extra information required, but not available, skipping
2012-07-19 17:37:19+0100 [-] The queue is now empty - stopping the scriptrunner.
2012-07-19 17:37:19+0100 [-] --- Emptied the scripts queue in 0.00 seconds---

To post a comment you must log in.
113. By Matt Fischer

code review comments

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'accomplishments/daemon/api.py'
2--- accomplishments/daemon/api.py 2012-07-17 17:02:45 +0000
3+++ accomplishments/daemon/api.py 2012-07-19 18:32:20 +0000
4@@ -97,7 +97,7 @@
5
6 @staticmethod
7 def run_a_subprocess(command):
8- # Commented out this debug message, as it creates lots of junk,
9+ # Commented out this debug message, as it creates lots of junk,
10 # and is not needed for common troubleshooting
11 # log.msg("Running subprocess command: " + str(command))
12 pprotocol = SubprocessReturnCodeProtocol()
13@@ -184,20 +184,20 @@
14 log.msg("The folder is shared, with: %s" % ", ".join(
15 shared_to))
16 return
17-
18+
19 self.parent._refresh_share_data()
20
21 # XXX let's rewrite this to use deferreds explicitly
22 @defer.inlineCallbacks
23 def start_scriptrunner(self):
24-
25+
26 # More info on scripts_state can be found in __init__
27 if self.scripts_state is RUNNING:
28 # Aborting this call - scriptrunner is already working.
29 return
30-
31+
32 self.scripts_state = RUNNING
33-
34+
35 uid = os.getuid()
36 # Is the user currently logged in and running a gnome session?
37 # XXX use deferToThread
38@@ -254,9 +254,9 @@
39 # XXX all parent calls should be refactored out of the AsyncAPI class
40 # to keep the code cleaner and the logic more limited to one particular
41 # task
42-
43+
44 queuesize = len(self.parent.scripts_queue)
45-
46+
47 log.msg("--- Starting Running Scripts - %d items on the queue ---" % (queuesize))
48 timestart = time.time()
49 if not self.parent.test_mode:
50@@ -265,7 +265,7 @@
51 while queuesize > 0:
52 accomID = self.parent.scripts_queue.popleft()
53 log.msg("Running %s, left on queue: %d" % (accomID, queuesize-1))
54-
55+
56 # First ensure that the acccomplishemt has not yet completed.
57 # It happens that the .asc file is present, but we miss the
58 # signal it triggers - so here we can re-check if it is not
59@@ -278,6 +278,8 @@
60 scriptpath = self.parent.get_acc_script_path(accomID)
61 if scriptpath is None:
62 log.msg("...No script for this accomplishment, skipping")
63+ elif not self.parent._is_all_extra_information_available(accomID):
64+ log.msg("...Extra information required, but not available, skipping")
65 else:
66 # There is a script for this accomplishmend, so run it
67 exitcode = yield self.run_a_subprocess([scriptpath])
68@@ -292,14 +294,14 @@
69 log.msg("...Could not get extra-information")
70 else:
71 log.msg("...Error code %d" % exitcode)
72-
73+
74 # New queue size is determined on the very end, since accomplish()
75 # might have added something new to the queue.
76 queuesize = len(self.parent.scripts_queue)
77
78-
79+
80 log.msg("The queue is now empty - stopping the scriptrunner.")
81-
82+
83 os.environ = oldenviron
84
85 # XXX eventually the code in this method will be rewritten using
86@@ -819,38 +821,57 @@
87 f = open(os.path.join(extrainfodir, item), 'w') #will trunkate the file, in case it exist
88 f.write(data)
89 f.close()
90- else:
91+ else:
92 #file would be empty, remove it instead
93 os.remove(os.path.join(extrainfodir, item))
94-
95+
96+ # Returns True if all extra information is available for an accom,
97+ # False otherwise
98+ def _is_all_extra_information_available(self, accomID):
99+ info_reqd = self.get_acc_needs_info(accomID)
100+ if len(info_reqd) == 0:
101+ return True
102+
103+ collection = self._coll_from_accomID(accomID)
104+ if not collection:
105+ return False
106+
107+ for info in info_reqd:
108+ ei = self.get_extra_information(collection, info)
109+ if not ei[0][info]:
110+ log.msg("%s is missing for %s, is_all_extra_information_available returning False" % (info, accomID))
111+ return False
112+
113+ return True
114+
115 def invalidate_extra_information(self,extrainfo):
116 """
117 .. warning::
118 This function is deprecated.
119-
120+
121 This function was used to remove all trophies that were accomplished using given extra-information. For example, if I used launchpad-email userA@mail.com and then switched to userB@mail.com, it was useful to call this function to remove all trophies that were awarded to userA@mail.com. However, since 0.2 throphies may not be deleted automatically under no circumstances, this function **does nothing** now.
122-
123- Args:
124+
125+ Args:
126 * **extrainfo** - (str) the extra-information field that is no more valid (e.g. launchpad-email)
127 """
128 pass
129-
130+
131 def get_extra_information(self, coll, item):
132 """
133 .. note::
134 This function is particularly sensitive - accomplishment scripts use it to fetch credentials they need.
135-
136+
137 This function returns extra-information's value, as set by user. It also provides it with translated label of this extra-information.
138-
139+
140 Args:
141 * **coll** - (str) the name of collection that needs this item. Depending on this, different label may be returned, if collections provide different extrainformation details.
142 * **item** - (str) the name of requested item.
143-
144+
145 Returns:
146 * **dict(str:str)** - output is wrapped in a dictionary:
147 - *item* - the value of this item. (e.g. ``"askubuntu-user-url" : "askubuntu.com/users/12345/nickname"``).
148 - **label** - a translated label, as provided by chosen collection.
149-
150+
151 Example:
152 >>> acc.get_extra_information("ubuntu-community","launchpad-email")
153 {"launchpad-email" : "user@host.org", "label" : "Adres e-mail uzywany do logowania w portalu Launchpad"}
154@@ -862,13 +883,13 @@
155 """
156 extrainfopath = os.path.join(self.trophies_path, ".extrainformation/")
157 authfile = os.path.join(extrainfopath, item)
158-
159+
160 if not self.get_collection_exists(coll):
161 log.msg("No such collection:" + coll)
162 return None
163-
164+
165 label = self.accDB[coll]['extra-information'][item]['label']
166-
167+
168 try:
169 f = open(authfile, "r")
170 data = f.read()
171@@ -877,9 +898,9 @@
172 #print "No data."
173 final = [{item : "", "label" : label}]
174 return final
175-
176+
177 # =================================================================
178-
179+
180 def reload_accom_database(self):
181 """
182 This is the function that builds up the *accDB* accomplishments database. It scans all accomplishment installation directories (as set in the config file), looks for all installed collections, and caches all accomplishments' data in memory. If a translated .accomplishment file is available, it's contents are loaded instead.
183@@ -1364,32 +1385,36 @@
184
185 def list_collections(self):
186 return [col for col in self.accDB if self.accDB[col]['type'] == 'collection']
187-
188+
189 # ====== Scriptrunner functions ======
190-
191+
192 def run_script(self,accomID):
193 if not self.get_acc_exists(accomID):
194 return
195 self.run_scripts([accomID])
196-
197- def run_scripts(self,which=None):
198- if which == None:
199- to_schedule = self.list_unlocked_not_completed()
200- elif type(which) is int or type(which) is bool or type(which) is dbus.Boolean:
201- log.msg("Note: This call to run_scripts is incorrect, run_scripts no more takes an int as an argument (it takes - optionally - a list of accomID to run their scripts)")
202- to_schedule = self.list_unlocked_not_completed()
203- else:
204- if len(which) is 0: # am empty list
205- return
206+
207+ def run_scripts(self, which=None):
208+ if isinstance(which, list):
209 to_schedule = which
210+ elif which == None:
211+ to_schedule = self.list_unlocked_not_completed()
212+ else:
213+ log.msg("Note: This call to run_scripts is incorrect, run_scripts takes (optionally) a list of accomID to run their scripts")
214+ to_schedule = self.list_unlocked_not_completed()
215+
216+ if len(to_schedule) == 0:
217+ log.msg("No scripts to run, returning without starting "\
218+ "scriptrunner")
219+ return
220+
221 log.msg("Adding to scripts queue: %s " % (str(to_schedule)))
222 for i in to_schedule:
223 if not i in self.scripts_queue:
224 self.scripts_queue.append(i)
225 self.asyncapi.start_scriptrunner()
226-
227+
228 # ====== Viewer-specific functions ======
229-
230+
231 def build_viewer_database(self):
232 accs = self.list_accomplishments()
233 db = []
234
235=== modified file 'accomplishments/daemon/tests/tests.py'
236--- accomplishments/daemon/tests/tests.py 2012-07-17 17:02:45 +0000
237+++ accomplishments/daemon/tests/tests.py 2012-07-19 18:32:20 +0000
238@@ -8,15 +8,17 @@
239 import subprocess
240 import ConfigParser
241 import datetime
242+import time
243+from collections import deque
244
245 sys.path.insert(0, os.path.join(os.path.split(__file__)[0], "../../.."))
246 from accomplishments.daemon import app, api
247
248 # future tests:
249 # create extra information files - marked for removal in the code
250-# run scripts/runscript
251 # get published status
252-# invalidate extra information
253+# publish and unpublish trophies online
254+# get_share_name, get_share_id
255
256 # These tests will modify the user's envrionment, outside of the test
257 # dir and so are not written/skipped:
258@@ -134,6 +136,39 @@
259 del os.environ['ACCOMPLISHMENTS_ROOT_DIR']
260 shutil.rmtree(self.td)
261
262+ def test_run_scripts(self):
263+ self.util_copy_accomp(self.accomp_dir, "third")
264+ a = api.Accomplishments(None, None, True)
265+
266+ # pass in None
267+ self.assertEqual(a.run_scripts(None), None)
268+ self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
269+ self.assertEqual(a.scripts_queue.popleft(),
270+ "%s/third" % self.ACCOMP_SET)
271+
272+ # pass in a bad arg
273+ self.assertEqual(a.run_scripts(122), None)
274+ self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
275+ self.assertEqual(a.scripts_queue.popleft(),
276+ "%s/third" % self.ACCOMP_SET)
277+
278+ # pass in a specific item
279+ self.assertEqual(a.run_scripts(["%s/third" % self.ACCOMP_SET]), None)
280+ self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
281+ self.assertEqual(a.scripts_queue.popleft(),
282+ "%s/third" % self.ACCOMP_SET)
283+
284+ def test_run_script(self):
285+ self.util_copy_accomp(self.accomp_dir, "third")
286+ a = api.Accomplishments(None, None, True)
287+ self.assertEqual(a.run_script("%s/wrong" % self.ACCOMP_SET), None)
288+ self.assertEqual(a.scripts_queue, deque([]))
289+ self.assertEqual(a.run_script("wrong"), None)
290+ self.assertEqual(a.scripts_queue, deque([]))
291+
292+ self.assertEqual(a.run_script("%s/third" % self.ACCOMP_SET), None)
293+ self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
294+
295 def test_get_acc_date_completed(self):
296 self.util_remove_all_accomps(self.accomp_dir)
297 self.util_copy_accomp(self.accomp_dir, "first")

Subscribers

People subscribed via source and target branches