Condenses read-only and writable GooglePlay into a single "Edit" type

mozapkpublisher/check_rollout.py

@@ -9,7 +9,7 @@
 
 from argparse import ArgumentParser
 from mozapkpublisher.common.googleplay import add_general_google_play_arguments, \
-    ReadOnlyGooglePlay, GooglePlayConnection
+    GooglePlayConnection, GooglePlayEdit
 
 DAY = 24 * 60 * 60
 
@@ -40,13 +40,13 @@ def main():
                         type=int, default=7)
     config = parser.parse_args()
 
-    google_play = ReadOnlyGooglePlay.create(GooglePlayConnection.open(
+    with GooglePlayEdit.transaction(GooglePlayConnection.open(
         config.service_account,
         config.google_play_credentials_file.name
-    ), 'org.mozilla.firefox')
-    for (release, age) in check_rollout(google_play, config.days):
-        print('fennec {} is on staged rollout at {}% but it shipped {} days ago'.format(
-              release['name'], int(release['userFraction'] * 100), int(age / DAY)))
+    ), 'org.mozilla.firefox', True) as google_play:
+        for (release, age) in check_rollout(google_play, config.days):
+            print('fennec {} is on staged rollout at {}% but it shipped {} days ago'.format(
+                  release['name'], int(release['userFraction'] * 100), int(age / DAY)))
 
 
 __name__ == '__main__' and main()

mozapkpublisher/common/googleplay.py

@@ -111,10 +111,14 @@ def connection_for_options(contact_google_play, service_account, credentials_fil
         return MockGooglePlayConnection()
 
 
-class ReadOnlyGooglePlay:
-    """Read-only access to the Google Play store
+class GooglePlayEdit:
+    """Represents an "edit" to an app on the Google Play store
 
-    Create an instance by calling ReadOnlyGooglePlay.create() instead of using the constructor
+    Create an instance by calling GooglePlayEdit.transaction(), instead of using the
+    constructor. This can optionally handle committing the transaction when the "with" block
+    ends.
+
+    E.g.: `with GooglePlayEdit.transaction() as google_play:`
     """
 
     def __init__(self, edit_resource, edit_id, package_name):
@@ -131,26 +135,6 @@ def get_track_status(self, track):
         logger.debug('Track "{}" has status: {}'.format(track, response))
         return response
 
-    @staticmethod
-    def create(connection, package_name):
-        edit_resource = connection.get_edit_resource()
-        edit_id = edit_resource.insert(body={}, packageName=package_name).execute()['id']
-        return ReadOnlyGooglePlay(edit_resource, edit_id, package_name)
-
-
-class WritableGooglePlay(ReadOnlyGooglePlay):
-    """Read-write access to the Google Play store
-
-    Create an instance by calling WritableGooglePlay.transaction(), instead of using the
-    constructor. This will automatically handle committing the transaction when the "with" block
-    ends.
-
-    E.g.: `with WritableGooglePlay.transaction() as google_play:`
-    """
-
-    def __init__(self, edit_resource, edit_id, package_name):
-        super().__init__(edit_resource, edit_id, package_name)
-
     def upload_apk(self, apk_path):
         logger.info('Uploading "{}" ...'.format(apk_path))
         try:
@@ -222,14 +206,14 @@ def update_whats_new(self, language, apk_version_code, whats_new):
 
     @staticmethod
     @contextmanager
-    def transaction(connection, package_name, do_not_commit=False):
+    def transaction(connection, package_name, commit):
         edit_resource = connection.get_edit_resource()
         edit_id = edit_resource.insert(body={}, packageName=package_name).execute()['id']
-        google_play = WritableGooglePlay(edit_resource, edit_id, package_name)
+        google_play = GooglePlayEdit(edit_resource, edit_id, package_name)
         yield google_play
-        if do_not_commit:
-            logger.warning('Transaction not committed, since `do_not_commit` was set')
-        else:
+        if commit:
             edit_resource.commit(editId=edit_id, packageName=package_name)
             logger.info('Changes committed')
             logger.debug('edit_id "{}" for "{}" has been committed'.format(edit_id, package_name))
+        else:
+            logger.warning('Transaction not committed, since `commit` was `False`')

mozapkpublisher/push_apk.py

@@ -6,7 +6,7 @@
 from mozapkpublisher.common import googleplay, main_logging
 from mozapkpublisher.common.apk import add_apk_checks_arguments, extract_and_check_apks_metadata
 from mozapkpublisher.common.exceptions import WrongArgumentGiven
-from mozapkpublisher.common.googleplay import WritableGooglePlay, connection_for_options
+from mozapkpublisher.common.googleplay import GooglePlayEdit, connection_for_options
 
 logger = logging.getLogger(__name__)
 
@@ -72,8 +72,7 @@ def push_apk(
     # by package name here.
     split_apk_metadata = _split_apk_metadata_per_package_name(apks_metadata_per_paths)
     for (package_name, apks_metadata) in split_apk_metadata.items():
-        with WritableGooglePlay.transaction(connection, package_name,
-                                            do_not_commit=not commit) as google_play:
+        with GooglePlayEdit.transaction(connection, package_name, commit) as google_play:
             for path, metadata in apks_metadata_per_paths.items():
                 google_play.upload_apk(path)
 

mozapkpublisher/test/common/test_googleplay.py

@@ -12,7 +12,7 @@
 from mozapkpublisher.common import googleplay
 from mozapkpublisher.common.exceptions import WrongArgumentGiven
 from mozapkpublisher.common.googleplay import add_general_google_play_arguments, \
-    WritableGooglePlay, MockGooglePlayConnection, ReadOnlyGooglePlay, GooglePlayConnection, \
+    GooglePlayEdit, MockGooglePlayConnection, GooglePlayConnection, \
     connection_for_options
 from mozapkpublisher.test import does_not_raise
 
@@ -55,35 +55,26 @@ def edit_resource_mock():
     return edit_resource
 
 
-def test_read_only_google_play_no_commit_transaction():
+def test_google_play_edit_no_commit_transaction():
     connection = MockGooglePlayConnection()
     mock_edits_resource = MagicMock()
     connection.get_edit_resource = lambda: mock_edits_resource
-    ReadOnlyGooglePlay.create(connection, 'package.name')
+    with GooglePlayEdit.transaction(connection, 'package.name', False) as _:
+        pass
 
     mock_edits_resource.commit.assert_not_called()
 
 
-def test_writable_google_play_commit_transaction():
+def test_google_play_edit_commit_transaction():
     connection = MockGooglePlayConnection()
     mock_edits_resource = MagicMock()
     connection.get_edit_resource = lambda: mock_edits_resource
-    with WritableGooglePlay.transaction(connection, 'package.name') as _:
+    with GooglePlayEdit.transaction(connection, 'package.name', True) as _:
         pass
 
     mock_edits_resource.commit.assert_called_with(editId=ANY, packageName='package.name')
 
 
-def test_writable_google_play_argument_do_not_commit_transaction():
-    connection = MockGooglePlayConnection()
-    mock_edits_resource = MagicMock()
-    connection.get_edit_resource = lambda: mock_edits_resource
-    with WritableGooglePlay.transaction(connection, 'package.name', do_not_commit=True) as _:
-        pass
-
-    mock_edits_resource.commit.assert_not_called()
-
-
 def test_get_track_status(edit_resource_mock):
     release_data = {
         "releases": [{
@@ -108,7 +99,7 @@ def test_get_track_status(edit_resource_mock):
 
     edit_resource_mock.tracks().get.reset_mock()
 
-    google_play = ReadOnlyGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
     assert google_play.get_track_status(track='production') == release_data
     edit_resource_mock.tracks().get.assert_called_once_with(
         editId=1,
@@ -123,7 +114,7 @@ def test_upload_apk_returns_files_metadata(edit_resource_mock):
     }
     edit_resource_mock.apks().upload.reset_mock()
 
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
     google_play.upload_apk(apk_path='/path/to/dummy.apk')
     edit_resource_mock.apks().upload.assert_called_once_with(
         editId=google_play._edit_id,
@@ -141,7 +132,7 @@ def test_upload_apk_errors_out(edit_resource_mock, http_status_code):
         # https://github.com/googleapis/google-api-python-client/blob/ffea1a7fe9d381d23ab59048263c631cc2b45323/googleapiclient/errors.py#L41
         content=b'{"error": {"errors": [{"reason": "someRandomReason"}] } }',
     )
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
 
     with pytest.raises(HttpError):
         google_play.upload_apk(apk_path='/path/to/dummy.apk')
@@ -169,14 +160,14 @@ def test_upload_apk_does_not_error_out_when_apk_is_already_published(edit_resour
         resp={'status': '403'},
         content=content_bytes,
     )
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
 
     with expectation:
         google_play.upload_apk(apk_path='/path/to/dummy.apk')
 
 
 def test_update_track(edit_resource_mock):
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
 
     google_play.update_track('alpha', ['2015012345', '2015012347'])
     edit_resource_mock.tracks().update.assert_called_once_with(
@@ -209,14 +200,14 @@ def test_update_track(edit_resource_mock):
 
 @pytest.mark.parametrize('invalid_percentage', (-1, 101))
 def test_update_track_should_refuse_wrong_percentage(edit_resource_mock, invalid_percentage):
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
 
     with pytest.raises(WrongArgumentGiven):
         google_play.update_track('rollout', ['2015012345', '2015012347'], invalid_percentage)
 
 
 def test_update_listings(edit_resource_mock):
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
 
     google_play.update_listings(
         'en-GB',
@@ -237,7 +228,7 @@ def test_update_listings(edit_resource_mock):
 
 
 def test_update_whats_new(edit_resource_mock):
-    google_play = WritableGooglePlay(edit_resource_mock, 1, 'dummy_package_name')
+    google_play = GooglePlayEdit(edit_resource_mock, 1, 'dummy_package_name')
 
     google_play.update_whats_new('en-GB', '2015012345', 'Check out this cool feature!')
     edit_resource_mock.apklistings().update.assert_called_once_with(

mozapkpublisher/test/test_check_rollout.py

@@ -21,7 +21,7 @@ def set_up_mocks(_requests_mock, tracks):
     _requests_mock.head('https://archive.mozilla.org/pub/mobile/releases/{}/SHA512SUMS'.format('62.0'),
                         status_code=404)
 
-    google_play_mock = create_autospec(googleplay.ReadOnlyGooglePlay)
+    google_play_mock = create_autospec(googleplay.GooglePlayEdit)
     google_play_mock.get_track_status.return_value = tracks
     return google_play_mock
 

mozapkpublisher/test/test_push_apk.py

@@ -30,7 +30,7 @@
 
 @pytest.fixture
 def writable_google_play_mock():
-    return create_autospec(googleplay.WritableGooglePlay)
+    return create_autospec(googleplay.GooglePlayEdit)
 
 
 def set_up_mocks(monkeypatch_, writable_google_play_mock_):
@@ -75,10 +75,10 @@ def _metadata(*args, **kwargs):
         }
 
     @contextmanager
-    def fake_transaction(_, __, do_not_commit):
+    def fake_transaction(_, __, ___):
         yield writable_google_play_mock_
 
-    monkeypatch_.setattr(googleplay.WritableGooglePlay, 'transaction', fake_transaction)
+    monkeypatch_.setattr(googleplay.GooglePlayEdit, 'transaction', fake_transaction)
     monkeypatch_.setattr('mozapkpublisher.push_apk.extract_and_check_apks_metadata', _metadata)
 
 

mozapkpublisher/test/test_update_apk_description.py

@@ -15,8 +15,8 @@
 
 
 def test_update_apk_description_force_locale(monkeypatch):
-    google_play_mock = create_autospec(googleplay.WritableGooglePlay)
-    monkeypatch.setattr(googleplay, 'WritableGooglePlay', lambda _, __, ___: google_play_mock)
+    google_play_mock = create_autospec(googleplay.GooglePlayEdit)
+    monkeypatch.setattr(googleplay, 'GooglePlayEdit', lambda _, __, ___: google_play_mock)
     monkeypatch.setattr(store_l10n, '_translations_per_google_play_locale_code', {
         'google_play_locale': {
             'title': 'Firefox for Android',

mozapkpublisher/update_apk_description.py

@@ -5,15 +5,15 @@
 
 from argparse import ArgumentParser
 from mozapkpublisher.common import googleplay, store_l10n
-from mozapkpublisher.common.googleplay import connection_for_options, WritableGooglePlay
+from mozapkpublisher.common.googleplay import connection_for_options, GooglePlayEdit
 
 logger = logging.getLogger(__name__)
 
 
 def update_apk_description(package_name, force_locale, commit, service_account, google_play_credentials_file,
                            contact_google_play):
     connection = connection_for_options(contact_google_play, service_account, google_play_credentials_file)
-    with WritableGooglePlay.transaction(connection, package_name, do_not_commit=not commit) as google_play:
+    with GooglePlayEdit.transaction(connection, package_name, commit) as google_play:
         moz_locales = [force_locale] if force_locale else None
         l10n_strings = store_l10n.get_translations_per_google_play_locale_code(package_name, moz_locales)
         create_or_update_listings(google_play, l10n_strings)