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
Contributor

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 tests
  • I added or improved docs for my feature
    • Swagger (including mage do-the-swag)
    • Error codes
    • New config options (including adding them to config.yml.saml and running mage generate-docs)
# 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 tests~~ * [ ] ~~I added or improved docs for my feature~~ * [ ] ~~Swagger (including `mage do-the-swag`)~~ * [ ] ~~Error codes~~ * [ ] ~~New config options (including adding them to `config.yml.saml` and running `mage generate-docs`)~~
konrad requested changes 2022-09-26 12:33:38 +00:00
konrad left a comment
Owner

Can you fix the lint errors?

Can you fix the lint errors?
@ -483,2 +483,4 @@
}
list.IsFavorite = favs[list.ID]
list.Subscription, err = GetSubscription(s, SubscriptionEntityList, list.ID, a)
Owner

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.
LucaBernstein marked this conversation as resolved
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from f1ca12dce9 to b6dbc334b2 2022-09-26 13:01:37 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from b6dbc334b2 to f0fc5e65ee 2022-09-26 14:10:42 +00:00 Compare
konrad reviewed 2022-09-26 16:15:50 +00:00
@ -479,0 +480,4 @@
for _, l := range lists {
listIds = append(listIds, l.ID)
}
subscriptions, err := GetSubscriptions(s, SubscriptionEntityList, listIds, a)
Owner

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.
LucaBernstein marked this conversation as resolved
@ -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) {
Owner

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?
Author
Contributor

done.

done.
LucaBernstein marked this conversation as resolved
konrad requested changes 2022-09-26 16:16:16 +00:00
konrad left a comment
Owner

Already looking better.

Already looking better.
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from f0fc5e65ee to 896d22847c 2022-09-28 09:01:54 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from 896d22847c to ac51451ab5 2022-09-28 09:08:11 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from ac51451ab5 to 033f19284d 2022-09-28 10:17:26 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from 033f19284d to 11b5739725 2022-09-28 11:06:24 +00:00 Compare
LucaBernstein requested review from konrad 2022-09-28 11:17:27 +00:00
konrad requested changes 2022-09-28 13:14:26 +00:00
konrad left a comment
Owner

Already much better!

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())
Owner

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.
Author
Contributor

will initialize the map on error

will initialize the map on error
konrad marked this conversation as resolved
@ -231,0 +232,4 @@
if err != nil {
return nil, err
}
for _, sub := range subs {
Owner

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 ```
Author
Contributor

Nice, thanks!

Nice, thanks!
Author
Contributor

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.
konrad marked this conversation as resolved
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from 11b5739725 to b28759a11d 2022-09-28 13:20:31 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from b28759a11d to cbcc1ad972 2022-09-28 13:20:57 +00:00 Compare
LucaBernstein requested review from konrad 2022-09-28 13:23:53 +00:00
konrad approved these changes 2022-09-28 16:10:00 +00:00
konrad left a comment
Owner

Looks good, should be ready to merge once CI passes.

Looks good, should be ready to merge once CI passes.
konrad scheduled this pull request to auto merge when all checks succeed 2022-09-28 16:10:24 +00:00
Owner

Can you fix the tests?

Can you fix the tests?
konrad canceled auto merging this pull request when all checks succeed 2022-09-28 17:39:29 +00:00
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from cbcc1ad972 to 96c79de2a3 2022-09-28 19:36:06 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from 96c79de2a3 to 8d5daa6c9a 2022-09-28 19:38:13 +00:00 Compare
LucaBernstein reviewed 2022-09-28 19:38:24 +00:00
@ -231,0 +232,4 @@
if err != nil || len(subs) == 0 {
return nil, err
}
for _, sub := range subs {
Author
Contributor

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 :)
konrad requested changes 2022-09-28 20:22:32 +00:00
konrad left a comment
Owner

Looks like tests are still failing :/

Looks like tests are still failing :/
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from 8d5daa6c9a to bfe3f96728 2022-09-28 20:38:52 +00:00 Compare
Author
Contributor

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

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
LucaBernstein requested review from konrad 2022-09-28 20:40:01 +00:00
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from bfe3f96728 to 3fe3ee491d 2022-09-28 20:43:08 +00:00 Compare
LucaBernstein force-pushed fix-namespaces-list-subscribe-view from 3fe3ee491d to 6b686e7843 2022-09-28 21:04:37 +00:00 Compare
konrad approved these changes 2022-09-29 09:49:02 +00:00
konrad merged commit 3adfeb3b34 into main 2022-09-29 09:49:29 +00:00
konrad deleted branch fix-namespaces-list-subscribe-view 2022-09-29 09:49:29 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#1254
No description provided.