https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34631

Marcel de Rooy <[email protected]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA

--- Comment #67 from Marcel de Rooy <[email protected]> ---
Looking here again. Your perseverance is exemplary btw.
Did you see Nicks comment about URL manipulation. What did you do with that?
In general this code does not yet look ready for a push. Some observations.

[% UNLESS ( my_branch_as_staff && my_branch_as_staff != c.branchcode ) %]
I am seeing this construct a few times. An unless with an AND does not always
read that easy. Are you sure this is what you mean?
The fact that this variable is filled rather complex (see below), makes this
check hard to understand.

+my $my_branch_as_staff =
+    C4::Context->preference("IndependentBranchesAdditionalcontents")
+    && !C4::Context->IsSuperLibrarian()
+    ? C4::Context->userenv()->{'branch'}
+    : undef;
Variable name is unclear. Do we need a current branch and some additional
permission variable ? The not superlibrarian condition looks weird too. If I am
superlib, it is false. Should it be pref true OR superlib ?

     my $additional_content = Koha::AdditionalContents->find($id);
+    $additional_content = undef
+        if ( $my_branch_as_staff
+        && $additional_content
+        && C4::Context->userenv()->{'branch'} ne
$additional_content->branchcode );
This looks suboptimal. You probably could prevent a db hit here in some cases
and adjust the search condition otherwise?

+    if ($my_branch_as_staff) {
+        @ids = grep {
+            my $id                 = $_;
+            my $additional_content = Koha::AdditionalContents->find($id);
+            defined $additional_content->branchcode
+                && $additional_content->branchcode eq
C4::Context->userenv()->{'branch'};
+        } @ids;
+    }
+    if (@ids) {
+        try {
+            Koha::Database->new->schema->txn_do(
+                sub {
+                    my $contents = Koha::AdditionalContents->search( { id =>
\@ids } );
Suboptimal too. Fetching records twice.

-- 
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

Reply via email to