OpenLP

Merge lp://qastaging/~trb143/openlp/reporting into lp://qastaging/openlp

  1. reporting
  2. Merge into trunk
Proposed by Tim Bentley
Status: Merged
Merged at revision: 2700
Proposed branch: lp://qastaging/~trb143/openlp/reporting
Merge into: lp://qastaging/openlp
Diff against target: 402 lines (+218/-28)
6 files modified
openlp/core/common/settings.py (+1/-0)
openlp/plugins/songs/lib/openlyricsxml.py (+1/-1)
openlp/plugins/songs/lib/songcompare.py (+3/-3)
openlp/plugins/songs/reporting.py (+106/-0)
openlp/plugins/songs/songsplugin.py (+27/-8)
tests/functional/openlp_core_ui/test_servicemanager.py (+80/-16)
To merge this branch: bzr merge lp://qastaging/~trb143/openlp/reporting
Reviewer Review Type Date Requested Status
Raoul Snyman Approve
Review via email: mp+309375@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2016-10-25.

Description of the change

My dad needed a report of all the songs on their database, they had 1800.
Made this into a reporting option and cleaned up the menu.
Fixed some errors spotted as well

Fixed issues and comments
1800 songs takes about 3 secs to run on my i7

lp:~trb143/openlp/reporting (revision 2701)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1801/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1712/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1650/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1406/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/996/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/1064/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/932/
[SUCCESS] https://ci.openlp.io/job/Branch-05c-Code_Analysis2/93/

To post a comment you must log in.
Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

1. There's a "csv" module, use it.
2. I should be able to specify the file name, not just where to save it to.
3. Have you tested this with > 1000 songs? How long does it take? Some sort of progress window necessary?
4. You call it a report internally, but you're not very specific for the user. Rather call it a "Song List Report".

More comments inline.

review: Needs Fixing
Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal

See my inline comments about your test doc stings and comments.

And also see my inline comments with regard to the Unicode literals. They're not required, and seem only to be implemented to ease porting from py2! https://www.python.org/dev/peps/pep-0414/#proposal

Are there any tests for the new module/method you've addded:
reporting.py
on_tools_report_song_list_triggered

Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal

Still missing tests for your new module. Guess its up to superfly if he's happy with that.

Also, just a few inline comments

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Couple extra things:

1. The save dialog says "output folder" or something, just make it say "Save file"
2. There's no list of file extensions in the dialog, and no default file name and/or extension.

review: Needs Fixing
Revision history for this message
Raoul Snyman (raoul-snyman) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.