stephpy / timeline

Standalone library to make a timeline.
MIT License
89 stars 20 forks source link

Action::setCreatedAt() vs Timeline::setCreatedAt() #42

Closed sadortun closed 9 years ago

sadortun commented 9 years ago

Salut Stéphane,

There is a setCreatedAt() method in both the Action and Timeline class.

Since we start to use the timeline bundle some time after the project we had to generate the timeline for the various actions.

$spy = $this->get('spy_timeline.action_manager');

foreach ($users as $user)
{
    $subject  = $spy->findOrCreateComponent($user);
    $action = $spy->create($subject, 'user_register');
    $action->setCreatedAt($user->getCreatedAt());
    $spy->updateAction($action);
}

The eaxample above correctly set the createdAt field for actions, but not in the timelines. Since the Timelines are sorted using the Timeline::createdAt all the actions are not sorted correctly.

I would make one of theses changes to the Timeline class

    /**
     * {@inheritdoc}
     */
    public function setAction(ActionInterface $action)
    {
        $this->action = $action;

        if($action->getCreatedAt())
                $this->setCreatedAt($action->getCreatedAt());

        return $this;
    }

Or


class AbstractTimelineManager
{
// ....

    public function createAndPersist(ActionInterface $action, ComponentInterface $subject, $context = 'GLOBAL', $type = TimelineInterface::TYPE_TIMELINE)
    {
        /** @var TimelineInterface $timeline */
        $timeline = new $this->timelineClass();

        $timeline
            ->setType($type)
            ->setAction($action)
            ->setContext($context)
            ->setSubject($subject)
        ;

      if ($action->getCreatedAt())
                $timeline->setCreatedAt($action->getCreatedAt());

        $this->objectManager->persist($timeline);
    }
}

Let me know what you think about this and ill make a PR

stephpy commented 9 years ago

Hi,

I understand your use case but it could not be the same for everyone.

CreatedAt field should represent the time where the entity was created. In your case, you're cheating and put a custom createdAt field in action & timelineClass.

IMO, the action->createdAt and timeline->createAt could be different. If you want to have a referencial time, you could add an other one field to the Action and Timeline entity but i'm not sure it's a good thing to use createdAt for this.

sadortun commented 9 years ago

Hi,

I agree that createdAt is probably not the right field to change, but we should probably have a way to add actions 'back in time'

A more typical use case would be for example if a user post need to be moderated, you will probably want to display the action on the timeline "User published an article on YYYY-MM-DD" where the date is the actual time the post was submitted, and not the date where the moderator approved it.

Thanks, Samuel

stephpy commented 9 years ago

Hi,

Yes, it's true, but it seems to be related to your domain. We can't do it for any project which use TimelineBundle/Timeline. IMO, You should override the timeline/action class or the manager in your side then doing your business, we can't provide it to everyone.

Thanks ;)

sadortun commented 9 years ago

I had a second look at this issue since its still searching for a solution... Looking at the QueryBuilder, i think it would be able to override the QueryBuilder or the TimelineManager to return the actions sorted with a custom entity field ?

Thanks for your input, Samuel

stephpy commented 9 years ago

Hi,

Yes, I guess you should do it with a custom field then override QB or Managers.

sadortun commented 9 years ago

Thanks, i'll give it a try!