Issue 5500078: Added a sample for email settings showing POP settings updation
|
Created:
12 years, 4 months ago by gunjansharma Modified:
12 years, 4 months ago CC:
gdata-python-client-library-contributors_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 28
Patch Set 2 : addressed first set of comments #
Total comments: 32
Patch Set 3 : Comments set2 resolved. #
Total comments: 12
Patch Set 4 : Set 3 resolved #Patch Set 5 : catching more error codes #MessagesTotal messages: 14
Add more comments around the email settings parts, the parts we want developers to understand. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:17: """Sample to demonstrate EmailSettings API's POP settings.""" s/Sample to demonstrate/Demonstrates/ What does this sample _do_? http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:18: One blank line, not two. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:27: SCOPE = ['https://apps-apis.google.com/a/feeds/emailsettings/2.0/', s/SCOPE/SCOPES/everywhere http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:27: SCOPE = ['https://apps-apis.google.com/a/feeds/emailsettings/2.0/', Aren't the scopes defined as constants in the client lib? If so, use those instead. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:46: def Authorize(self): Should this method be called from __init__? I'm not sure clients should be required to call these methods in order before calling update. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:61: 'Press a key after authorization.') If I run this, and I press "a", nothing happens. I think you meant to say "press Return" http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:66: def SetPopSettings(self): Choose a more appropriate name here, given that the class name is PopSettings. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:77: """Runs the sample using an instance of PopSettings.""" What does the main method _do_? What does it do to a domain or user account? http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:79: # Parse command line options Superfluous comment. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:93: # Parse options Superfluous comment. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:102: while not consumer_key: Command-line flag, or raw_input, not both. We don't want to have to explain this as part of the sample. Instead, we want developers to focus on the email settings parts. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:113: except gdata.client.RequestError, e: Life might be easier if you created your own exceptions, and raised those instead. Clients would then have a better understanding of what was going on. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:122: return Superfluous return statement.
Sign in to reply to this message.
http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:17: """Sample to demonstrate EmailSettings API's POP settings.""" On 2011/12/26 17:37:33, Vic Fryzel wrote: > s/Sample to demonstrate/Demonstrates/ > What does this sample _do_? Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:18: On 2011/12/26 17:37:33, Vic Fryzel wrote: > One blank line, not two. Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:27: SCOPE = ['https://apps-apis.google.com/a/feeds/emailsettings/2.0/', On 2011/12/26 17:37:33, Vic Fryzel wrote: > s/SCOPE/SCOPES/everywhere Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:27: SCOPE = ['https://apps-apis.google.com/a/feeds/emailsettings/2.0/', On 2011/12/26 17:37:33, Vic Fryzel wrote: > Aren't the scopes defined as constants in the client lib? If so, use those > instead. Not email settings's. More over 'apps' has all the sort of Provisioning scopes be it groups or organization unit provisioning. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:46: def Authorize(self): On 2011/12/26 17:37:33, Vic Fryzel wrote: > Should this method be called from __init__? I'm not sure clients should be > required to call these methods in order before calling update. This was pointed to me in my previous samples that it should be called from outside. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:61: 'Press a key after authorization.') On 2011/12/26 17:37:33, Vic Fryzel wrote: > If I run this, and I press "a", nothing happens. I think you meant to say > "press Return" Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:66: def SetPopSettings(self): On 2011/12/26 17:37:33, Vic Fryzel wrote: > Choose a more appropriate name here, given that the class name is PopSettings. Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:77: """Runs the sample using an instance of PopSettings.""" On 2011/12/26 17:37:33, Vic Fryzel wrote: > What does the main method _do_? What does it do to a domain or user account? Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:79: # Parse command line options On 2011/12/26 17:37:33, Vic Fryzel wrote: > Superfluous comment. Done! Idea was to be consistent with other samples. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:93: # Parse options On 2011/12/26 17:37:33, Vic Fryzel wrote: > Superfluous comment. Done! http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:102: while not consumer_key: On 2011/12/26 17:37:33, Vic Fryzel wrote: > Command-line flag, or raw_input, not both. We don't want to have to explain > this as part of the sample. Instead, we want developers to focus on the email > settings parts. Done! http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:113: except gdata.client.RequestError, e: On 2011/12/26 17:37:33, Vic Fryzel wrote: > Life might be easier if you created your own exceptions, and raised those > instead. Clients would then have a better understanding of what was going on. Done. http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:122: return On 2011/12/26 17:37:33, Vic Fryzel wrote: > Superfluous return statement. Done!
Sign in to reply to this message.
http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:27: SCOPE = ['https://apps-apis.google.com/a/feeds/emailsettings/2.0/', On 2011/12/27 13:39:59, gunjansharma wrote: > On 2011/12/26 17:37:33, Vic Fryzel wrote: > > Aren't the scopes defined as constants in the client lib? If so, use those > > instead. > Not email settings's. More over 'apps' has all the sort of Provisioning scopes > be it groups or organization unit provisioning. Then what is this line doing here: http://code.google.com/p/gdata-python-client/source/browse/src/gdata/apps/ema... And this line: http://code.google.com/p/gdata-python-client/source/browse/src/gdata/apps/mul... Either you should be using those values, or those values are incorrect and should be fixed. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:18: Enables pop settings for all the domain users for all emails and What does "Enables pop settings" mean? Re-word this sentence to be easier to read. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:19: archieves Google Mail's copy. archives http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:28: 2 blank lines after imports http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:39: """Constructor for the PopSettingsException object.""" Create a new PopSettingsException with the appropriate message. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:40: Error.__init__(self, message) Use super() http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:42: print 'Invalid Domain' Do not print when constructing this exception. This exception has methods for getting a message, and the Python interpreter will print the message for you. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:83: """Updates POP settings for all the domain users.""" s/the domain/of the domain's/, same elsewhere http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:93: """Updates POP settings for all domain users using EmailSettings API.""" s/EmailSettings/Email Settings/everywhere http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:109: if option == '--consumer_UpdateDomainUsersPopSettingskey': Did you test this? I don't see how this could work. Please test the entire sample after this round of comments is addressed. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:118: print ('python emailsettings_pop_settings.py' This usage string does not need to be defined twice. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:121: return Above, you sys.exit(2). Yet here, you return (essentially exiting with 0). Which is correct? http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:125: try: Do you need two try blocks?
Sign in to reply to this message.
Small comments. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:33: class Error(Exception): Why does this superclass exist? http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:37: class PopSettingsException(Error): Docstring http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:51: """Constructor for PopSettings object. Create a new PopSettings object configured for a domain. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:116: if not consumer_key or not consumer_secret or not domain: Optional, but I prefer: if not (consumer_key and consumer_secret and domain): http://en.wikipedia.org/wiki/De_Morgan's_laws
Sign in to reply to this message.
http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/1/samples/apps/emailsettings_pop_s... samples/apps/emailsettings_pop_settings.py:27: SCOPE = ['https://apps-apis.google.com/a/feeds/emailsettings/2.0/', On 2011/12/27 15:59:35, Vic Fryzel wrote: > On 2011/12/27 13:39:59, gunjansharma wrote: > > On 2011/12/26 17:37:33, Vic Fryzel wrote: > > > Aren't the scopes defined as constants in the client lib? If so, use those > > > instead. > > Not email settings's. More over 'apps' has all the sort of Provisioning scopes > > be it groups or organization unit provisioning. > > Then what is this line doing here: > http://code.google.com/p/gdata-python-client/source/browse/src/gdata/apps/ema... > > And this line: > http://code.google.com/p/gdata-python-client/source/browse/src/gdata/apps/mul... > > Either you should be using those values, or those values are incorrect and > should be fixed. Actually as I said above It includes all the provisioning scopes and not email settings, Before they all used the same scopes as 'https://apps-apis.google.com/a/feeds' but now they all use different scope. So the those values in the email settings API are incorrect. Look here http://code.google.com/p/gdata-python-client/source/browse/src/gdata/gauth.py... http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:18: Enables pop settings for all the domain users for all emails and On 2011/12/27 15:59:35, Vic Fryzel wrote: > What does "Enables pop settings" mean? > > Re-word this sentence to be easier to read. Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:19: archieves Google Mail's copy. On 2011/12/27 15:59:35, Vic Fryzel wrote: > archives Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:28: On 2011/12/27 15:59:35, Vic Fryzel wrote: > 2 blank lines after imports Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:33: class Error(Exception): On 2011/12/27 16:30:38, Ali Afshar wrote: > Why does this superclass exist? Removed http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:37: class PopSettingsException(Error): On 2011/12/27 16:30:38, Ali Afshar wrote: > Docstring Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:39: """Constructor for the PopSettingsException object.""" On 2011/12/27 15:59:35, Vic Fryzel wrote: > Create a new PopSettingsException with the appropriate message. Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:40: Error.__init__(self, message) On 2011/12/27 15:59:35, Vic Fryzel wrote: > Use super() Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:42: print 'Invalid Domain' On 2011/12/27 15:59:35, Vic Fryzel wrote: > Do not print when constructing this exception. This exception has methods for > getting a message, and the Python interpreter will print the message for you. Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:51: """Constructor for PopSettings object. On 2011/12/27 16:30:38, Ali Afshar wrote: > Create a new PopSettings object configured for a domain. Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:83: """Updates POP settings for all the domain users.""" On 2011/12/27 15:59:35, Vic Fryzel wrote: > s/the domain/of the domain's/, same elsewhere Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:93: """Updates POP settings for all domain users using EmailSettings API.""" On 2011/12/27 15:59:35, Vic Fryzel wrote: > s/EmailSettings/Email Settings/everywhere Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:109: if option == '--consumer_UpdateDomainUsersPopSettingskey': On 2011/12/27 15:59:35, Vic Fryzel wrote: > Did you test this? I don't see how this could work. > > Please test the entire sample after this round of comments is addressed. Done! Sorry function name got copied 2 times. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:116: if not consumer_key or not consumer_secret or not domain: On 2011/12/27 16:30:38, Ali Afshar wrote: > Optional, but I prefer: > if not (consumer_key and consumer_secret and domain): > http://en.wikipedia.org/wiki/De_Morgan%27s_laws Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:118: print ('python emailsettings_pop_settings.py' On 2011/12/27 15:59:35, Vic Fryzel wrote: > This usage string does not need to be defined twice. Done. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:121: return On 2011/12/27 15:59:35, Vic Fryzel wrote: > Above, you sys.exit(2). Yet here, you return (essentially exiting with 0). > Which is correct? Done. Used sys.exit(2) as we want to leave the program. http://codereview.appspot.com/5500078/diff/4001/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:125: try: On 2011/12/27 15:59:35, Vic Fryzel wrote: > Do you need two try blocks? Done.
Sign in to reply to this message.
http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:18: Enables POP for all of the domain's users for all emails and s/emails/messages http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:19: archives Google Mail's copy. s/Google Mail/Gmail http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:43: """Demonstrates updation of POP settings for all of the domain's users.""" Sample demonstrating how to enable POP for all of a domain's users. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:96: using Email Settings API. using the http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:104: sys.exit(2) Please help me understand why are you exiting with exit code 2 instead of exit code 1? Same below. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:129: raise PopSettingsException('Error: Invalid Domain') s/Error: // here and below
Sign in to reply to this message.
http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... File samples/apps/emailsettings_pop_settings.py (right): http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:18: Enables POP for all of the domain's users for all emails and On 2011/12/28 18:07:05, Vic Fryzel wrote: > s/emails/messages Done. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:19: archives Google Mail's copy. On 2011/12/28 18:07:05, Vic Fryzel wrote: > s/Google Mail/Gmail Done. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:43: """Demonstrates updation of POP settings for all of the domain's users.""" On 2011/12/28 18:07:05, Vic Fryzel wrote: > Sample demonstrating how to enable POP for all of a domain's users. Done. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:96: using Email Settings API. On 2011/12/28 18:07:05, Vic Fryzel wrote: > using the Done. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:104: sys.exit(2) On 2011/12/28 18:07:05, Vic Fryzel wrote: > Please help me understand why are you exiting with exit code 2 instead of exit > code 1? Same below. Changed to 1. I had this feeling anything non zero is normal termination and abnormal otherwise. http://codereview.appspot.com/5500078/diff/3004/samples/apps/emailsettings_po... samples/apps/emailsettings_pop_settings.py:129: raise PopSettingsException('Error: Invalid Domain') On 2011/12/28 18:07:05, Vic Fryzel wrote: > s/Error: // here and below Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
|
