fix(namespaces): add list subscriptions #1254
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#1254
Loading…
Reference in New Issue
No description provided.
Delete Branch "LucaBernstein/api:fix-namespaces-list-subscribe-view"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Add list subscriptions to namespaces call to enable frontend to show subscription state correctly.
See original issue on GitHub: https://github.com/go-vikunja/frontend/issues/75
See roadmap item: https://my.vikunja.cloud/tasks/16503
Checklist
I added or improved testsI added or improved docs for my featureSwagger (includingmage do-the-swag
)Error codesNew config options (including adding them toconfig.yml.saml
and runningmage generate-docs
)Can you fix the lint errors?
@ -483,2 +483,4 @@
}
list.IsFavorite = favs[list.ID]
list.Subscription, err = GetSubscription(s, SubscriptionEntityList, list.ID, a)
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.
f1ca12dce9
tob6dbc334b2
b6dbc334b2
tof0fc5e65ee
@ -479,0 +480,4 @@
for _, l := range lists {
listIds = append(listIds, l.ID)
}
subscriptions, err := GetSubscriptions(s, SubscriptionEntityList, listIds, a)
Please use
listIDs
- that's alread pre-populated with all list IDs, no need to do the same work again.@ -253,2 +253,4 @@
}
// 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) {
Can you unify this function with
GetSubscription
? So thatGetSubscription
only callsGetSubscriptions
with one parameter?done.
Already looking better.
f0fc5e65ee
to896d22847c
896d22847c
toac51451ab5
ac51451ab5
to033f19284d
033f19284d
to11b5739725
Already much better!
@ -478,1 +478,4 @@
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())
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
@ -231,0 +232,4 @@
if err != nil {
return nil, err
}
for _, sub := range subs {
subs
may benil
if there are no subscription. The whole expression here could be simplified to something likeNice, thanks!
go linter was actually hinting that for nil
subs
,len(subs)
would default to 0.11b5739725
tob28759a11d
b28759a11d
tocbcc1ad972
Looks good, should be ready to merge once CI passes.
Can you fix the tests?
cbcc1ad972
to96c79de2a3
96c79de2a3
to8d5daa6c9a
@ -231,0 +232,4 @@
if err != nil || len(subs) == 0 {
return nil, err
}
for _, sub := range subs {
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 :)Looks like tests are still failing :/
8d5daa6c9a
tobfe3f96728
Seems like this is a flapping test due to multiple results being returned from the query and the logic taking either one or the other.
I added some logic to take precedence for exact matches over parents
bfe3f96728
to3fe3ee491d
3fe3ee491d
to6b686e7843