ubuntu-touch-coreapps-reviewers team mailing list archive

[Andrew Hayzen <ahayzen@xxxxxxxxx>]

Review: Needs Fixing

Looks good so far :) Just a few inline comments...

L65, L75, L188 and possibly L140 - Could you remove this extra blank lines? (See PEP8 styling on this [0] - 2 lines for a class def and 1 for a method)
L102 - Is there any reason for renaming this var and method call? Could we keep it as add_to_playlist singular?

As I don't think jenkins runs for non-team members, I've manually run PEP8/pyflakes for you so you can see the errors [1]

0 - https://www.python.org/dev/peps/pep-0008/#blank-lines
1 - http://pastebin.ubuntu.com/10629706/

Diff comments:

> === modified file 'MusicPlaylists.qml'
> --- MusicPlaylists.qml	2015-01-21 00:10:33 +0000
> +++ MusicPlaylists.qml	2015-03-19 19:38:54 +0000
> @@ -93,6 +93,7 @@
>          delegate: Card {
>              id: playlistCard
>              coverSources: Playlists.getPlaylistCovers(model.name)
> +	    objectName: "playlistCardItem" + index
>              primaryText: model.name
>              secondaryText: i18n.tr("%1 song", "%1 songs", model.count).arg(model.count)
>  
> 
> === modified file 'common/SongsPage.qml'
> --- common/SongsPage.qml	2015-01-30 01:23:39 +0000
> +++ common/SongsPage.qml	2015-03-19 19:38:54 +0000
> @@ -593,6 +593,7 @@
>          id: removePlaylistDialog
>          Dialog {
>              id: dialogRemovePlaylist
> +	    objectName: "dialogRemovePlaylist"
>              // TRANSLATORS: this is a title of a dialog with a prompt to delete a playlist
>              title: i18n.tr("Permanently delete playlist?")
>              text: "("+i18n.tr("This cannot be undone")+")"
> @@ -602,6 +603,7 @@
>              Button {
>                  text: i18n.tr("Remove")
>                  color: styleMusic.dialog.confirmRemoveButtonColor
> +		objectName: "removePlaylistDialogRemoveButton"
>                  onClicked: {
>                      // removing playlist
>                      Playlists.removePlaylist(dialogRemovePlaylist.oldPlaylistName)
> 
> === modified file 'tests/autopilot/music_app/__init__.py'
> --- tests/autopilot/music_app/__init__.py	2015-01-19 17:13:12 +0000
> +++ tests/autopilot/music_app/__init__.py	2015-03-19 19:38:54 +0000
> @@ -75,6 +75,10 @@
>          return self.main_view.wait_select_single(
>              Dialog, objectName="dialogNewPlaylist")
>  
> +    def get_delete_playlist_dialog(self):
> +        return self.main_view.wait_select_single(
> +            Dialog, objectName="dialogRemovePlaylist")
> +
>      def get_now_playing_page(self):
>          return self.app.wait_select_single(MusicNowPlaying,
>                                             objectName="nowPlayingPage")
> @@ -208,6 +212,23 @@
>          return self.wait_select_single(
>              "CardView", objectName="playlistsCardView").count
>  
> +    def click_new_playlist_action(self):
> +            self.main_view.get_header().click_action_button("newPlaylistButton")
> +
> +    @click_object
> +    def click_playlist(self, i):
> +        return self.get_playlist(i)
> +
> +    def get_playlist(self, i):
> +        return (self.wait_select_single("Card",
> +                objectName="playlistCardItem" + str(i)))
> +
> +    def click_delete_playlist_action(self):
> +            self.main_view.get_header().click_action_button("deletePlaylist")
> +
> +    
> +
> +
>  
>  class MusicaddtoPlaylist(MusicPage):
>      """ Autopilot helper for add to playlist page """
> @@ -400,6 +421,9 @@
>  
>          self.swipping.wait_for(False)
>  
> +    

Could you remove this extra whitespace?

> +    
> +
>  
>  class Dialog(UbuntuUIToolkitCustomProxyObjectBase):
>      @click_object
> @@ -411,6 +435,11 @@
>          self.wait_select_single(
>              "TextField", objectName="playlistNameTextField").write(text)
>  
> +    @click_object
> +    def click_remove_playlist_dialog_remove_button(self):
> +        return self.wait_select_single(
> +            "Button", objectName="removePlaylistDialogRemoveButton")
> +
>  
>  class MainView(MainView):
>      """Autopilot custom proxy object for the MainView."""
> 
> === modified file 'tests/autopilot/music_app/tests/test_music.py'
> --- tests/autopilot/music_app/tests/test_music.py	2015-01-19 17:13:12 +0000
> +++ tests/autopilot/music_app/tests/test_music.py	2015-03-19 19:38:54 +0000
> @@ -449,13 +449,13 @@
>  
>          track.click_add_to_playlist_action()  # add track to queue
>  
> -        add_to_playlist_page = self.app.get_add_to_playlist_page()
> +        add_to_playlists_page = self.app.get_add_to_playlists_page()

Is there any reason for renaming this var and method call? Could we keep it as add_to_playlist singular?

>  
>          # get initial list view playlist count
> -        playlist_count = add_to_playlist_page.get_count()
> +        playlist_count = add_to_playlists_page.get_count()
>  
>          # click on New playlist button in header
> -        add_to_playlist_page.click_new_playlist_action()
> +        add_to_playlists_page.click_new_playlist_action()
>  
>          # get dialog
>          new_dialog = self.app.get_new_playlist_dialog()
> @@ -467,17 +467,17 @@
>          new_dialog.click_new_playlist_dialog_create_button()
>  
>          # verify playlist has been sucessfully created
> -        self.assertThat(add_to_playlist_page.get_count(),
> +        self.assertThat(add_to_playlists_page.get_count(),
>                          Eventually(Equals(playlist_count + 1)))
>  
> -        self.assertThat(add_to_playlist_page.get_playlist(0).name,
> +        self.assertThat(add_to_playlists_page.get_playlist(0).name,
>                          Equals("myPlaylist"))
>  
>          # select playlist to add song to
> -        add_to_playlist_page.click_playlist(0)
> +        add_to_playlists_page.click_playlist(0)
>  
>          # wait for add to playlist page to close
> -        add_to_playlist_page.visible.wait_for(False)
> +        add_to_playlists_page.visible.wait_for(False)
>  
>          # open playlists page
>          playlists_page = self.app.get_playlists_page()
> @@ -485,6 +485,62 @@
>          # verify song has been added to playlist
>          self.assertThat(playlists_page.get_count(), Equals(1))
>  
> +        
> +
> +    def test_select_and_delete_playlist(self):
> +        """tests deleting a playlist by creating a playlist, 
> +            selecting it, and then deleting it. """
> +
> +        # switch to playlist page
> +        playlists_page = self.app.get_playlists_page()
> +
> +        # get initial list view playlist count
> +        playlist_count = playlists_page.get_count()
> +
> +        # click on New playlist button in header
> +        playlists_page.click_new_playlist_action()
> +
> +        # get dialog
> +        new_dialog = self.app.get_new_playlist_dialog()
> +
> +        # input playlist name
> +        new_dialog.type_new_playlist_dialog_name("testDeletePlaylist")
> +
> +        # click on the create Button
> +        new_dialog.click_new_playlist_dialog_create_button()
> +
> +        # verify playlist has been sucessfully created
> +        self.assertThat(playlists_page.get_count(),
> +                        Eventually(Equals(playlist_count + 1)))
> +
> +        self.assertThat(playlists_page.get_playlist(0).primaryText,
> +                        Equals("testDeletePlaylist"))
> +
> +        # select the playlist that was just created
> +        playlists_page.click_playlist(0)
> +
> +        # click the delete icon
> +        playlists_page.click_delete_playlist_action()
> +
> +        # get dialog
> +        delete_dialog = self.app.get_delete_playlist_dialog()
> +
> +        # click on the remove Button
> +        delete_dialog.click_remove_playlist_dialog_remove_button()
> +
> +        playlists_page = self.app.get_playlists_page()
> +
> +        # verify that the playlist has been removed
> +        self.assertThat(playlists_page.get_count(), Equals(playlist_count))
> +
> +

Whitespace :)

> +        
> +
> +        
> +
> +
> +        
> +
>      def test_artists_tab_album(self):
>          """tests navigating to the Artists tab and playing an album"""
>  
> 


-- 
https://code.launchpad.net/~darran-kelinske/music-app/lp_bug_1428454/+merge/253566
Your team Music App Developers is subscribed to branch lp:music-app.