Notifications filter (PR #10119)

notifications-filter bug solved issue: https://meta.discourse.org/t/notifications-unread-only-filter/37621/32

GitHub

This looks like a good start - but it should include tests too. Both on ruby and the javascript side.

Why is this being set? I don’t see it referenced elsewhere.

I don’t think the changes in this file are worth comitting.

Hey, @eviltrout I did the changes as specified. just tests are left

Is this needed? I think it should automatically use this controller because it has the same name :thinking:

This is looking great! I left a few comments on some specific lines.

We also need some tests, and it looks like there are some merge conflicts to resolve.

It would be better if we can position this using CSS without adding a separator element. How about wrapping the notifications-filter in a <div>, and then setting it to 100% width with a border-bottom.

@Ahmedgagan just bumping this comment ^^

I think we can reduce some repetition here by doing something like

      notifications = Notification.visible
                     .where(user_id: user.id)
                     .includes(:topic)
                     .order(created_at: :desc)
     
     if params[:filter] == "read"
       notifications = notifications.where(read: true)
     elsif params[:filter] == "unread"
       notifications = notifications.where(read: false)
     end

Sure, ill try this.

Yes, i already changed this in my last commit.

If i remove this it gives this error

Assertion Failed: You're not allowed to have more than one controller property map to the same query param key, but both `userNotifications:filter` and `user-notifications:filter` map to `filter`. You can fix this by mapping one of the controller properties to a different query param key via the `as` config option, e.g. `filter: { as: 'other-filter' }

Sure, ill try this out