search

bpuig / laravel-subby

Laravel Plan and Subscriptions manager.
https://bpuig.github.io/laravel-subby
MIT License
102 stars 42 forks source link

Getting the default subscription for any subscriber #70

Closed boryn closed 3 years ago

boryn commented 3 years ago

On second thought and while developing a real project, I'd go with this solution for the default subscription tag. ->subscription() will work now independently of the tag used for the subscriber.

As mentioned before, we use different tag names for different types of subscribers. Now $user->subscription() and $company->subscription() will work universally without a need to find their corresponding subscription tag. And when someone wants to use different tags for the same type of subscriber, they should be always passed explicitly.

In the second commit I used 'subby.main_subscription_tag' for creating the new subscription.

bpuig commented 3 years ago

766e44c sounds good to me

1454948 makes me a bit suspicious, since can lead to problems of consistency. Wouldn't be better to retrieve first one only if there is one? And raise exception if there are more than one in database? But also, config is already there and does effectively that.

boryn commented 3 years ago

I'll come up with another approach

boryn commented 3 years ago

Ok, please have a look at 5313f7c73cd59b2ed3a61c79722e7d56faa10b61 (or just at modified files https://github.com/bpuig/laravel-subby/pull/70/files). Now it's backward compatible and fully secure.

bpuig commented 3 years ago

I appreciate your effort, but I need to think a bit more if this logic is necessary. Can you split this PR in two and I'll aprove 766e44c and I'll think a bit more about the other one. Thanks

boryn commented 3 years ago

Hi! I made a separate pull request for the default tag for the new subscription.

I will as well explain briefly when the need for picking the first subscription arised. In our system we have different subscribers: $user and $company, moreover companies have two types: constructor and maintanence.

We use different subscription tags for all these groups in order to later filter ending subscriptions with the scope getByTag(). This is useful for the reminder notifications. Each of the subscribers uses just one subscription and using a generic ->subscription() as proposed here would be very convenient for picking the right subscription. Otherwise all the developers need to remember to use the proper tag for each subscriber and pass it to ->subscription().

On the other hand we don't want to use the 'main' tag for all these groups as this would complicate picking up the expiring subscriptions.

As mentioned before, this solution is more versatile (if there is exactly one sub, pick this up) and if there are more, pick the one with the default 'main' tag. If 0, or not found the 'main', throw an exception.