Multiple notification filters #16101
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ncounter
changed the title
danidoni
force-pushed
the
notifications-multiple-filters
branch
from
940baf4
to
eb0b97a
Compare
ncounter
force-pushed
the
notifications-multiple-filters
branch
from
813b9f2
to
5e12679
Compare
danidoni
force-pushed
the
notifications-multiple-filters
branch
2 times, most recently
from
b555a0d
to
21c4027
Compare
ncounter
force-pushed
the
notifications-multiple-filters
branch
from
1fcf0fc
to
805dad8
Compare
danidoni
force-pushed
the
notifications-multiple-filters
branch
2 times, most recently
from
37f545e
to
dc969a2
Compare
danidoni
force-pushed
the
notifications-multiple-filters
branch
4 times, most recently
from
047556e
to
998aac8
Compare
ncounter
force-pushed
the
notifications-multiple-filters
branch
9 times, most recently
from
5ac4349
to
9f3f45e
Compare
ncounter
force-pushed
the
notifications-multiple-filters
branch
from
78efd5f
to
8815369
Compare
|
The counters are a bit confusing. Look at this screenshot, I filtered by
|
srbaker
previously approved these changes
May 13, 2024
hennevogel
requested changes
May 13, 2024
src/api/app/controllers/webui/users/notifications_controller.rb
Outdated
Show resolved
Hide resolved
src/api/app/controllers/webui/users/notifications_controller.rb
Outdated
Show resolved
Hide resolved
src/api/app/controllers/webui/users/notifications_controller.rb
Outdated
Show resolved
Hide resolved
saraycp
previously requested changes
May 13, 2024
src/api/spec/controllers/webui/users/notifications_controller_spec.rb
Outdated
Show resolved
Hide resolved
Yeah, that sounds like a bug |
Adapt the cleanup job using a scope
When we mark notifications as read or unread we get the authenticity_token and several other parameters. When using strong parameters, we have to explicitly permit them otheriwise the form is never going to work. By namespacing the notification params, we can just forget about those other params.
Only when one of the two status is selected [Read|Unread]. In case of both, the multiselection is disabled: it would be tricky to select multiple notifications with different status and mark them "Read" or "Unread" with no real criteria to decide.
danidoni
force-pushed
the
notifications-multiple-filters
branch
from
8b707b5
to
d2954bd
Compare
danidoni
pushed a commit
to hennevogel/open-build-service
that referenced
this pull request
May 20, 2024
This will be the foundation where we'll build a better openSUSE#16101. We decouple the read/unread status of a notification from it's type (which notifiable it associated to).
danidoni
pushed a commit
to hennevogel/open-build-service
that referenced
this pull request
May 21, 2024
This will be the foundation where we'll build a better openSUSE#16101. We decouple the read/unread status of a notification from it's type (which notifiable it associated to).
ncounter
pushed a commit
to hennevogel/open-build-service
that referenced
this pull request
May 22, 2024
This will be the foundation where we'll build a better openSUSE#16101. We decouple the read/unread status of a notification from it's type (which notifiable it associated to).
ncounter
pushed a commit
to hennevogel/open-build-service
that referenced
this pull request
May 22, 2024
This will be the foundation where we'll build a better openSUSE#16101. We decouple the read/unread status of a notification from it's type (which notifiable it associated to).
|
Superseded by #16183 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changed?
Before, there was a list of links, where every link was loading a different subset of notifications based on a query string parameter
type=..... It was possible to load one type of notifications at a time.I.e.:
After, there are a list of filter options (the same amount of before), but now they are checkboxes, part of a form that can be submitted by an
Applybutton; this submit loads a subset of notifications filtered by the criteria of the checkboxes selection, this time as a multiple flavors selection based onstatus/type/project/groupat the same time combining them with anANDamong the filter group, and with anORamong the internal filter group option.I.e.:
Note
By default, if no filter is selected at all, it returns the list of
Unreadnotifications only and it automatically check theUnreadfilter checkbox. It is intended to be so because at first, the user wants to read the Unread notifications only. Just in case the user selects theReadfilter checkbox and applies the filter, then the read notifications will be rendered too.Note 2
Now that potentially both Read and Unread notifications are selected and rendered, when that scenario happens, the notification action bar that contains the select all checkbox and Mark all as read|unread button is not visible, due to the risk of misunderstanding: it would not be a mark as but it would be a toggler of the state of each selected notification, which is seldom the desired behavior for a multiple selection.
Before
After