Allow customizing the model name

[AndrewRadev]

The Activity model comes from the gem, which means it's not very convenient to customize. I'd rather have a domain model in my app that I can extend. It's still somewhat possible to plug custom extensions with an initializer that monkey-patches the ::PublicActivity::Activity class, but this doesn't work well with autoloading and it has to be placed in a different location than all other models.

The change needed to support this is actually quite simple, the string ::PublicActivity::Activity is replaced with a config option. This should provide perfect backwards compatibility and a new hook to plug in your own class (in my case, class ::Activity < ::PublicActivity::Activity) that is a full-fledged AR model, owned by the app.

Let me know if you'd like me to add more tests or documentation, I wasn't sure how far I should go in that direction.

[coveralls]

Coverage increased (+0.0%) when pulling 407cf48 on knowableorg:custom-activity-class-name into 06237e4 on pokonski:master.

[farnoy]

Why do you need your own Activity class? What does it give you?

[AndrewRadev]

This is the current contents of the class, more or less:

class Activity < ::PublicActivity::Activity
  def self.unread
    where(read: false)
  end

  def unread?
    not read?
  end

  def created_on
    created_at.to_date
  end

  def mark_read!
    update_attributes!(read: true)
  end
end

The read column was added at some point, and it makes sense to add some logic to do with it in this class. You could even imagine keeping a read_at field to remember when it was marked as "read".

Even if we put these particular extensions aside, having the activity model external and unmodifiable makes it hard to find a place to fit this kind of logic. The library can't provide an interface for all use cases. And while for a lot of things it's possible to extend them from the outside (it's not necessary to use render_activity, for instance, you could generate the html as you'd like), the class that's used for the relations is fixed. current_user.activities_as_recipient would always return ::PublicActivity::Activity instances, so tacking on an .unread to it would never work. The only solution I see is to let the application provide its own Activity model.

[pokonski]

Hi Andrew! That is an interesting idea and the best approach in my opinion, too. 👍 Though one thing I see missing is we must make sure the custom Model inherits from PublicActivity::Activity. I can already see people forgetting this and destroying the functionality.

Not quite sure where we would add that restriction in the code. Any ideas?

[farnoy]

Unless you need different versions of Activity in one app (one with read column, the other with something_else) at the same time, you can just extend the Activity class with your own stuff, add custom columns and define methods like unread? and the associations will return this class (because it's the same one).

The library can't provide an interface for all use cases

Exactly why I'm reluctant about all of this.

[pokonski]

@farnoy has a valid point, too. This is how I personally extend the PublicActivity::Activity:

https://github.com/pokonski/public_activity/wiki/%5BHow-to%5D-Use-custom-fields-on-Activity#setup

[AndrewRadev]

Though one thing I see missing is we must make sure the custom Model inherits from PublicActivity::Activity. I can already see people forgetting this and destroying the functionality.

Yeah, that's true. It would make sense to add a hook somewhere to check if the model .kind_of? ::PublicActivity::Activity. What I don't like about it is that it's somewhat un-ruby-ish. Theoretically, there's no reason for you to extend ::PublicActivity::Activity, as long as you respect the same API.

One alternative would be to provide a ::PublicActivity::Lint model, much like what Rack does (http://rubydoc.info/github/rack/rack/master/Rack/Lint). This would, however, mean going through the project and figuring out how the Activity model is used, so I decided to skip it for now. Maybe the first option is fine to begin with anyway.

This is how I personally extend the PublicActivity::Activity:

Yes, and as I mentioned, I'm not a fan of this approach. It means that application code with potentially non-trivial business logic (methods that you would call often) is stuck in an initializer, outside of the app/models directory. I find this confusing -- if I see user.activities.unread, I'd look for the unread method in the Activity model, except it's in a library, and it's extended in a special place. There's some mental hoops to jump through to get to this, especially if you're not the one that implemented this.

A more practical problem is that putting it there means any changes require a restart of the server, as opposed to changes to classes in app/models. This ensures that you even have a different workflow when you extend activities, and I find this uncomfortable as well. It makes this domain logic "special", when I don't feel like it should be.

I think an interesting parallel could be made with devise. While it does provide tons of functionality to the User model, it expects you to define it yourself. If you imagine devise forcing you to use their model and extend it with a class_eval you can probably see how this would be inconvenient at best. Of course, devise does much more complicated things with the User model, not to mention it lets you have multiple models. I'm just drawing the parallel to give you some perspective on how I see the Activity model right now. It's not as volatile as a User (I may never ever have to add anything more to activities), but still, it's a piece of application code that has to be handled with care, outside of the normal coding flow.

Naturally, this will be a maintentance burden to you as owners of the project, so I'd understand if you reject the PR on these grounds. I don't feel it's a very risky change (in fact, I was very happy with how easy it was to make), but that's for you to decide. We can use our own fork for now and see how it goes.

[farnoy]

I think code extending the Activity class can be stored in app/models, or p_a can be adjusted to allow for that.

Also, when you inherit from us and we change something internal, you're doomed aswell. This would obviously be a dependency.

If your requirements are far higher than what we provide, then I'd recommend either forking this project or just taking parts into your app.

Your remark about devise is interesting indeed, but there's inherent complexity from that approach and I'm not sure if we want to dive into that.

We provide two modules of functionality that you can inherit: Common, Tracked. When you look at what devise is giving you, it's tons of strategies, model functionality, controllers, views, route & test helpers, so it makes sense for them to organise it with more granularity.

I think our approach is okay for what we'd like to stay a simple library and complicating this just isn't our desire.

[pokonski]

If your requirements are far higher than what we provide, then I'd recommend either forking this project or just taking parts into your app.

I don't agree. People forked a lot just to add some basic attributes and that was a bad way. That's why we added custom attributes documentation so people don't feel forking is the only way. And forks get out of date very fast.

I think our approach is okay for what we'd like to stay a simple library and complicating this just isn't our desire.

We haven't really consulted this ;) So it is your vision.

A more practical problem is that putting it there means any changes require a restart of the server, as opposed to changes to classes in app/models.

I agree that class_eval is more of a hack than a solution. Also a valid point about generating your own Activity model, like devise does. This idea I really like. Though it would require adding generators for that, so new users don't have to type it by hand.

[AndrewRadev]

I think code extending the Activity class can be stored in app/models, or p_a can be adjusted to allow for that.

If that's possible, that would be a reasonable solution to my issues with it. I'm not sure if autoloading could work, though, I think rails really expects you to define a class in the appropriately named file. Still, I haven't tried. I'd definitely accept a working solution along these lines instead of mine.

Your remark about devise is interesting indeed, but there's inherent complexity from that approach and I'm not sure if we want to dive into that.

We provide two modules of functionality that you can inherit: Common, Tracked. When you look at what devise is giving you, it's tons of strategies, model functionality, controllers, views, route & test helpers, so it makes sense for them to organise it with more granularity.

Yes, devise is a much more complex project and I'm sure they pay for the complexity in terms of maintenance costs. I mentioned it mostly for the sake of the mental model.

Also, when you inherit from us and we change something internal, you're doomed aswell. This would obviously be a dependency.

Yes, you're right. That's one reason it might make sense to have a Lint module that checks the API. Although, technically, changing the library can break things even now, since the model is still used in the app's controllers and views, even if it's untouched. Hard for me to say how much of a difference this addition would make. Application-library boundaries are always tricky.

[pokonski]

@AndrewRadev sorry for no reply for over six months, we are definitely implementing this one way or the other because you had a good idea.

We are in the process of rewriting p_a for 2.0 release which will be backwards incompatible and will allow for easier customization. Which is something your PR can help with.

[AndrewRadev]

No worries, I know how hard it is to keep up with maintenance of an open source project. Let me know if there's anything I can help out with.

[iBublik]

I just wanted to make PR like this and found this one. Hope this option will be available in the future, cause it'll be really great to use own model with other features provided by gem.

[jnraine]

@pokonski I'm working on an app that needed to customize PublicActivity::Activity model. It's possible but requires some workarounds (roughly, re-opening the class in an initializer).

I like @AndrewRadev suggestion here and wanted to ask if this pull request could be merged. If you are interested in this feature but the PR needs some revisions, I'd be happy to look into it. Thanks!

[pokonski]

Hey @jnraine, looks like @ur5us can also help with this, since it will require some adjustments after merging #341