fix(namespaces): add list subscriptions #1254

Merged
konrad merged 1 commits from LucaBernstein/api:fix-namespaces-list-subscribe-view into main 2022-09-29 09:49:29 +00:00
2 changed files with 45 additions and 10 deletions

View File

@ -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

If there's an error here subscriptions may be nil which means this will explode in line 491.

Please either make sure the subscriptions variable contains an empty map or return the error here.

If there's an error here `subscriptions` may be `nil` 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

will initialize the map on error
subscriptions = make(map[int64]*Subscription)
}
LucaBernstein marked this conversation as resolved Outdated

Please use listIDs - that's alread pre-populated with all list IDs, no need to do the same work again.

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

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 {

View File

@ -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

subs may be nil if there are no subscription. The whole expression here could be simplified to something like

if subs == nil || len(subs) == 0 {
	return nil, nil
}

return subs[0], nil
`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 ```

Nice, thanks!

Nice, thanks!

go linter was actually hinting that for nil subs, len(subs) would default to 0.

go linter was actually hinting that for nil `subs`, `len(subs)` would default to 0.

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 :)

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

Can you unify this function with GetSubscription? So that GetSubscription only calls GetSubscriptions with one parameter?

Can you unify this function with `GetSubscription`? So that `GetSubscription` only calls `GetSubscriptions` with one parameter?

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) {