openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

I've got a couple of comments below for you to take a look at.

Also, as previously noted, you need a code test before we'll accept your merge proposal. I'm afraid that manual testing, while nice, does not constitute a test for merging purposes.

Diff comments:

> === modified file 'openlp/plugins/songs/lib/mediaitem.py'
> --- openlp/plugins/songs/lib/mediaitem.py	2017-01-25 21:17:27 +0000
> +++ openlp/plugins/songs/lib/mediaitem.py	2017-06-04 14:21:07 +0000
> @@ -231,9 +231,13 @@
>  
>      def search_entire(self, search_keywords):
>          search_string = '%{text}%'.format(text=clean_string(search_keywords))
> -        return self.plugin.manager.get_all_objects(
> -            Song, or_(Song.search_title.like(search_string), Song.search_lyrics.like(search_string),
> -                      Song.comments.like(search_string)))
> +        return (self.plugin.manager.session.query(Song)

Why the extra set of brackets? Use \ for line continuation.

> +                .join(SongBookEntry, isouter=True)
> +                .join(Book, isouter=True)
> +                .filter(or_(Book.name.like(search_string), SongBookEntry.entry.like(search_string),
> +                            Song.search_title.like(search_string), Song.search_lyrics.like(search_string),
> +                            Song.comments.like(search_string), Song.alternate_title.like(search_string)))

search_title already contains a searchable form of alternate_title.

> +                .all())
>  
>      def on_song_list_load(self):
>          """


-- 
https://code.launchpad.net/~samothjtm/openlp/bug-1695587/+merge/325035
Your team OpenLP Core is subscribed to branch lp:openlp.