statistics: add tests, query and csv download button for librarian statistics #2661

vgranata · 2022-01-27T13:40:41Z

Adds tests for librarian statistics
Solves issue of user with more than one patron account (Impossible to access statistics from a patron linked to more than one organisation #2697)
Adds query: number of loan transactions by item location
Adds button to download csv file of specific queries
Changes name of queries labels and order output data (Statistics: better labels with translations #2675)

Co-Authored-by: Valeria Granata [email protected]

Why are you opening this PR?

https://tree.taiga.io/project/rero21-reroils/task/2371
Solves issue: #2697
Solves issue: #2675

How to test?

What command should I have to run to test your PR?
What should I test through the UI?

Code review check list

rero_ils/modules/stats/api.py

rero_ils/modules/stats/permissions.py

pronguen · 2022-03-17T10:13:08Z

I gave the "librarian" role to a patron (pid:46008). Then I cannot access the professional interface as this patron. This is working on the production.
- Observed with Firefox 98.0.1 (64-bit). It works on Chrome.

jma · 2022-03-21T06:14:38Z

rero_ils/modules/stats/cli.py

    """Extract the stats librarian for one year and store them in db.

    :param year: year of statistics
+    :param n: month up to which the statistics are calculated


n is not a good variable name. Please use something more explicit.

jma · 2022-03-21T06:16:15Z

rero_ils/modules/stats/cli.py

+                n += 1
+            else:
+                click.secho(f'ERROR: not a valid month')
+                return


You can use a click.Abort here: https://click.palletsprojects.com/en/7.x/api/#click.Abort

jma · 2022-03-21T06:16:54Z

rero_ils/modules/stats/cli.py

+            if n in range(1, 13):
+                n += 1
+            else:
+                click.secho(f'ERROR: not a valid month')


You can display the message in red: click.secho(f'ERROR: not a valid month', fg='red')

jma · 2022-03-21T06:18:44Z

rero_ils/modules/stats/permissions.py

+def filter_stat_by_librarian(record):
+    """Filter data for libraries of specific librarian/system_librarian.
+
+    :param record: Record to check.


What do yo mean by Record? I think this can be more explicit.

jma · 2022-03-21T06:20:41Z

rero_ils/modules/stats/serializers.py

@@ -28,38 +29,97 @@
 class StatCSVSerializer(CSVSerializer):
    """Mixin serializing records as JSON."""


Can you change this docstring?

jma · 2022-03-21T06:31:05Z

rero_ils/modules/stats/templates/rero_ils/stats_list.html

+    </tr>
+    <tr>
+      <td class="col-xs-12">
+        <a href="{{url_for('invenio_records_ui.stats', pid_value=rec._source.pid)}}">View statistics</a>


"View statistics" should be translated.

jma · 2022-03-21T06:31:36Z

rero_ils/modules/stats/templates/rero_ils/stats_list.html

+    </tr>
+    <tr>
+      <td>Loans of the transaction library by item location
+        <a class="btn btn-outline-secondary float-right" href="/stats/librarian/{{rec._source.pid}}/csv?query_id=1" role="button"><i class="fa fa-download"></i></a>


href can be replaced by an url_for.

jma · 2022-03-21T06:32:19Z

rero_ils/modules/stats/templates/rero_ils/stats_list.html

+    {%- endfor %}
+  </div>
+{% elif type=="librarian" %}
+  <table class="table table-hover border">


Should this table be responsive?

It is responsive.

jma · 2022-03-21T06:33:49Z

rero_ils/modules/stats/views.py

+@blueprint.route('/librarian/<record_pid>/csv')
+@check_logged_as_librarian
+def stats_librarian_queries(record_pid):
+    """Download specific statistic query into csv file."""


Can yo describe the parameter and the return value?

jma · 2022-03-21T06:36:07Z

rero_ils/modules/stats/views.py

+    """Download specific statistic query into csv file."""
+    queries = {'1': 'loans_of_transaction_library_by_item_location'}
+    query_id = request.args.get('query_id', None)
+    if query_id not in queries:


It seems that only 1 is allowed for the query_id parameter. If it is true this seems overkill.

It is done to foresee the implementation of other queries in the future. Should I change it ?

Is the usage url should be "/librarian/<record_pid>/csv?query_id=1" ?
Why not use exact term as query_id value ? ("/librarian/<record_pid>/csv?query_id= loans_of_transaction_library_by_item_location")

my option is that using "1" is less human relevant than a string argument " loans_of_transaction_library_by_item_location" when you read/write the URL.

…atistics Co-Authored-by: Valeria Granata <[email protected]>

zannkukai

Code seems correct. Great job 👏 . I just added some cosmetics comments/personnal reflexions

zannkukai · 2022-03-23T12:52:55Z

rero_ils/modules/stats/api.py

-        if 'librarian' in patron_data.roles:
-            for library_pid in patron_data.libraries:
-                library_pids.add(library_pid.pid)
+        if 'librarian' in current_librarian["roles"]:


You could use Patron.ROLE_LIBRARIAN instead of "librarian" string. This isn't a wrong implementation, just a good practice in my opinion.

zannkukai · 2022-03-23T12:54:52Z

rero_ils/modules/stats/api.py

+            for library in current_librarian.get('libraries', []):
+                library_pids.add(extracted_data_from_ref(library))


COSMETCI : better pythonic way

library_pids = set([extracted_data_from_ref(lib) for lib in current_librarian.get('libraries', [])])

zannkukai · 2022-03-23T12:55:22Z

rero_ils/modules/stats/api.py

-            organisation_libraries_pid = LibrariesSearch()\
-                .filter('term',
-                        organisation__pid=patron_data.organisation.pid)\
+        if 'system_librarian' in current_librarian["roles"]:


better to use Patron.ROLE_SYSTEM_LIBRARIAN

zannkukai · 2022-03-23T12:57:59Z

rero_ils/modules/stats/api.py

+                .filter(
+                    'term',
+                    organisation__pid=patron_organisation.pid)\


COSMETIC : single line ?

zannkukai · 2022-03-23T12:58:44Z

rero_ils/modules/stats/api.py

+            for s in libraries_search:
                library_pids.add(s.pid)


library_pids = library_pids.union(set([s.pid for s in libraries_search]))

or (to be tested)

library_pids = library_pids | set([s.pid for s in libraries_search])

zannkukai · 2022-03-23T13:08:56Z

rero_ils/modules/stats/serializers.py

+        if 'type' in record['metadata'] and\
+                record['metadata']['type'] == 'librarian':


if record['metadata'].get('type') == Patron.ROLE_LIBRARIAN:

zannkukai · 2022-03-23T13:10:40Z

rero_ils/modules/stats/serializers.py

+            headers = [key[0].upper()+key[1:].replace('_', ' ')
+                       for key in self.ordered_keys]


use capitalize() --> headers = [key.capitalize().replace('_', '') for key in self.ordered_keys]

zannkukai · 2022-03-23T13:14:04Z

rero_ils/modules/stats/views.py

+    :return: response object, the csv file
+    """
+    queries = {'1': 'loans_of_transaction_library_by_item_location'}
+    query_id = request.args.get('query_id', None)


maybe add an USAGE section into the function doc_string to explain query_id query string argument.

zannkukai · 2022-03-23T13:15:54Z

rero_ils/modules/stats/views.py

+    """Download specific statistic query into csv file."""
+    queries = {'1': 'loans_of_transaction_library_by_item_location'}
+    query_id = request.args.get('query_id', None)
+    if query_id not in queries:


Is the usage url should be "/librarian/<record_pid>/csv?query_id=1" ?
Why not use exact term as query_id value ? ("/librarian/<record_pid>/csv?query_id= loans_of_transaction_library_by_item_location")

my option is that using "1" is less human relevant than a string argument " loans_of_transaction_library_by_item_location" when you read/write the URL.

zannkukai · 2022-03-23T13:18:58Z

rero_ils/modules/stats/views.py

+    values = sorted(
+                dictionary,
+                key=lambda v: v['library']['name']
+            )
+    return values


merge into single instruction : return sorted(....)

… query Co-Authored-by: Valeria Granata <[email protected]>

github-actions bot added permissions labels Jan 27, 2022

vgranata marked this pull request as ready for review January 27, 2022 16:26

vgranata requested a review from jma January 27, 2022 16:26

jma requested changes Feb 1, 2022

View reviewed changes

rero_ils/modules/stats/api.py Outdated Show resolved Hide resolved

rero_ils/modules/stats/api.py Outdated Show resolved Hide resolved

rero_ils/modules/stats/permissions.py Outdated Show resolved Hide resolved

vgranata changed the title ~~statistics: add tests for librarian statistics~~ statistics: librarian statistics Feb 15, 2022

vgranata mentioned this pull request Feb 15, 2022

Impossible to access statistics from a patron linked to more than one organisation #2697

Closed

vgranata marked this pull request as draft February 16, 2022 07:40

vgranata force-pushed the grv-2371-tests-statistics-librarian branch 2 times, most recently from b6e2411 to aaf9d61 Compare February 16, 2022 08:12

vgranata changed the title ~~statistics: librarian statistics~~ statistics: add tests, query and csv download button for librarian statistics Feb 16, 2022

vgranata mentioned this pull request Feb 16, 2022

Statistics: better labels with translations #2675

Closed

PascalRepond added f: permissions Concerns the rights management and removed statistics labels Feb 18, 2022

vgranata force-pushed the grv-2371-tests-statistics-librarian branch from 1f7b295 to 81a3fde Compare March 1, 2022 16:38

github-actions bot added the f: statistics Related to the usage statistics either for pricing or for the libaries reports label Mar 1, 2022

vgranata force-pushed the grv-2371-tests-statistics-librarian branch 2 times, most recently from c879d8c to 8f03db5 Compare March 10, 2022 14:49

vgranata marked this pull request as ready for review March 10, 2022 14:50

vgranata requested a review from jma March 10, 2022 14:51

jma approved these changes Mar 21, 2022

View reviewed changes

vgranata requested review from BadrAly and Garfield-fr March 21, 2022 08:51

statistics: add tests, query and csv download button for librarian st…

89a3900

…atistics Co-Authored-by: Valeria Granata <[email protected]>

vgranata force-pushed the grv-2371-tests-statistics-librarian branch from 8f03db5 to 8f8a916 Compare March 21, 2022 15:13

vgranata requested a review from zannkukai March 23, 2022 10:58

zannkukai approved these changes Mar 23, 2022

View reviewed changes

statistics: change name of queries labels, order results and optimise…

bb03aed

… query Co-Authored-by: Valeria Granata <[email protected]>

vgranata force-pushed the grv-2371-tests-statistics-librarian branch from 8f8a916 to bb03aed Compare March 24, 2022 15:02

PascalRepond merged commit 9956ab2 into rero:staging Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: add tests, query and csv download button for librarian statistics #2661

statistics: add tests, query and csv download button for librarian statistics #2661

vgranata commented Jan 27, 2022 •

edited

Loading

pronguen commented Mar 17, 2022 •

edited

Loading

jma Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

vgranata Mar 21, 2022

jma Mar 21, 2022

jma Mar 21, 2022

vgranata Mar 21, 2022 •

edited

Loading

zannkukai Mar 23, 2022

zannkukai left a comment

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

zannkukai Mar 23, 2022

		@@ -28,38 +29,97 @@
		class StatCSVSerializer(CSVSerializer):
		"""Mixin serializing records as JSON."""

		for library in current_librarian.get('libraries', []):
		library_pids.add(extracted_data_from_ref(library))

		if 'type' in record['metadata'] and\
		record['metadata']['type'] == 'librarian':

		headers = [key[0].upper()+key[1:].replace('_', ' ')
		for key in self.ordered_keys]

statistics: add tests, query and csv download button for librarian statistics #2661

statistics: add tests, query and csv download button for librarian statistics #2661

Conversation

vgranata commented Jan 27, 2022 • edited Loading

Why are you opening this PR?

How to test?

Code review check list

pronguen commented Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgranata Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zannkukai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgranata commented Jan 27, 2022 •

edited

Loading

pronguen commented Mar 17, 2022 •

edited

Loading

vgranata Mar 21, 2022 •

edited

Loading