slub / slub_events

EXT:slub_events
GNU General Public License v3.0
2 stars 15 forks source link

Output of location not consistent #63

Closed twaurisch closed 3 years ago

twaurisch commented 3 years ago

To output the location in an email or .ics file a $helper array is used. Ignoring that the same code is used in different places (I'll do a refactoring) I want to focus on these two code blocks.

SubscriberController.php:234

if (is_object($event->getLocation())) {
    if (is_object($event->getLocation()->getParent()->current())) {
        $helper['location'] = $event->getLocation()->getParent()->current()->getName() . ', ';
        $helper['locationics'] = $this->foldline($event->getLocation()->getParent()->current()->getName()) . ', ';
    }
    $helper['location'] .= $event->getLocation()->getName();
    $helper['locationics'] .= $this->foldline($event->getLocation()->getName());
}

and SubscriberController.php:611

if (is_object($event->getLocation())) {
    if (is_object($event->getLocation()->getParent()->current())) {
        $helper['location'] = $event->getLocation()->getParent()->current()->getName() . ', ';
        $helper['locationics'] =
            $this->foldline($event->getLocation()->getParent()->current()->getName()) . ', ';
    }
    $helper['location'] = $event->getLocation()->getName();
    $helper['locationics'] = $this->foldline($event->getLocation()->getName());
}

The only difference is the missing string concatenation in the second block. It seems that a concatenation was intended, because of the comma three lines earlier, but the complete gets overridden by the child.

Whats' the wanted behavior?

If the concatenation is wanted another problem comes up, because in the first code block two folded lines were put together.

IMHO: The content composition for locations and their parents should happen in the template, also the foldline-method should be a ViewHelper.

I can do this, if you give me you opinion. Maybe it also makes sense to put $helper into a separate class or remove $helper and put some logic into the Event::class

albig commented 3 years ago

The code is even three times in SubscriberController.php. It's on line 413, too. ;-)

In the deleteAction() and in beOnlineSurveyAction() the location is not important at all. That's why nobody missed the parent location till now.

The concatenation was the intention:

Library 
  |--> Meeting Room

Branch Library
  |--> Meeting Room

Should result in "Library, Meeting Room" and "Branch Library, Meeting Room". Otherwise the location "Meeting Room" would be not unique.