fix(namespaces): add list subscriptions #1254
|
@ -476,12 +476,22 @@ func addListDetails(s *xorm.Session, lists []*List, a web.Auth) (err error) {
|
|||
return err
|
||||
}
|
||||
|
||||
subscriptions, err := GetSubscriptions(s, SubscriptionEntityList, listIDs, a)
|
||||
if err != nil {
|
||||
log.Errorf("An error occurred while getting list subscriptions for a namespace item: %s", err.Error())
|
||||
konrad marked this conversation as resolved
Outdated
|
||||
subscriptions = make(map[int64]*Subscription)
|
||||
}
|
||||
LucaBernstein marked this conversation as resolved
Outdated
konrad
commented
Please use Please use `listIDs` - that's alread pre-populated with all list IDs, no need to do the same work again.
|
||||
|
||||
for _, list := range lists {
|
||||
// Don't override the favorite state if it was already set from before (favorite saved filters do this)
|
||||
LucaBernstein marked this conversation as resolved
Outdated
konrad
commented
This is doing a db request in a loop. That's very inefficent. Please change it so that it loads all subscription status upfront and then map them together. This is doing a db request in a loop. That's very inefficent. Please change it so that it loads all subscription status upfront and then map them together.
|
||||
if list.IsFavorite {
|
||||
continue
|
||||
}
|
||||
list.IsFavorite = favs[list.ID]
|
||||
|
||||
if subscription, exists := subscriptions[list.ID]; exists {
|
||||
list.Subscription = subscription
|
||||
}
|
||||
}
|
||||
|
||||
if len(fileIDs) == 0 {
|
||||
|
|
|
@ -228,28 +228,53 @@ func getSubscriberCondForEntity(entityType SubscriptionEntityType, entityID int6
|
|||
// that task, if there is none it will look for a subscription on the list the task belongs to and if that also
|
||||
// doesn't exist it will check for a subscription for the namespace the list is belonging to.
|
||||
func GetSubscription(s *xorm.Session, entityType SubscriptionEntityType, entityID int64, a web.Auth) (subscription *Subscription, err error) {
|
||||
subs, err := GetSubscriptions(s, entityType, []int64{entityID}, a)
|
||||
if err != nil || len(subs) == 0 {
|
||||
return nil, err
|
||||
}
|
||||
if sub, exists := subs[entityID]; exists {
|
||||
konrad marked this conversation as resolved
Outdated
konrad
commented
`subs` may be `nil` if there are no subscription. The whole expression here could be simplified to something like
```go
if subs == nil || len(subs) == 0 {
return nil, nil
}
return subs[0], nil
```
LucaBernstein
commented
Nice, thanks! Nice, thanks!
LucaBernstein
commented
go linter was actually hinting that for nil go linter was actually hinting that for nil `subs`, `len(subs)` would default to 0.
LucaBernstein
commented
I am actually back at the I am actually back at the `for _, sub := range` construct, because of the map. For the mapping it is not an array where I can simply access the first element, but I need the first value from the map available. @konrad please have a look again :)
|
||||
return sub, nil // Take exact match first, if available
|
||||
}
|
||||
for _, sub := range subs {
|
||||
return sub, nil // For parents, take next available
|
||||
}
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
// GetSubscriptions returns a map of subscriptions to a set of given entity IDs
|
||||
func GetSubscriptions(s *xorm.Session, entityType SubscriptionEntityType, entityIDs []int64, a web.Auth) (listsToSubscriptions map[int64]*Subscription, err error) {
|
||||
u, is := a.(*user.User)
|
||||
if !is {
|
||||
return
|
||||
}
|
||||
|
||||
if err := entityType.validate(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
subscription = &Subscription{}
|
||||
cond := getSubscriberCondForEntity(entityType, entityID)
|
||||
exists, err := s.
|
||||
var entitiesFilter builder.Cond
|
||||
for _, eID := range entityIDs {
|
||||
if entitiesFilter == nil {
|
||||
LucaBernstein marked this conversation as resolved
Outdated
konrad
commented
Can you unify this function with Can you unify this function with `GetSubscription`? So that `GetSubscription` only calls `GetSubscriptions` with one parameter?
LucaBernstein
commented
done. done.
|
||||
entitiesFilter = getSubscriberCondForEntity(entityType, eID)
|
||||
continue
|
||||
}
|
||||
entitiesFilter = entitiesFilter.Or(getSubscriberCondForEntity(entityType, eID))
|
||||
}
|
||||
|
||||
var subscriptions []*Subscription
|
||||
err = s.
|
||||
Where("user_id = ?", u.ID).
|
||||
And(cond).
|
||||
Get(subscription)
|
||||
if !exists {
|
||||
And(entitiesFilter).
|
||||
Find(&subscriptions)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
subscription.Entity = subscription.EntityType.String()
|
||||
|
||||
return subscription, err
|
||||
listsToSubscriptions = make(map[int64]*Subscription)
|
||||
for _, sub := range subscriptions {
|
||||
sub.Entity = sub.EntityType.String()
|
||||
listsToSubscriptions[sub.EntityID] = sub
|
||||
}
|
||||
return listsToSubscriptions, nil
|
||||
}
|
||||
|
||||
func getSubscribersForEntity(s *xorm.Session, entityType SubscriptionEntityType, entityID int64) (subscriptions []*Subscription, err error) {
|
||||
|
|
If there's an error here
subscriptions
may benil
which means this will explode in line 491.Please either make sure the
subscriptions
variable contains an empty map or return the error here.will initialize the map on error