[frontend] Fix default subscription switch #4384
Merged
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
|
@adrianschroeter JFYI^ |
evanrolfe
reviewed
Jan 22, 2018
| @@ -0,0 +1,2 @@ | |||
| $('#subscriptions-form').replaceWith('<%= escape_javascript(render(partial: 'webui/subscriptions/subscriptions_form')) %>'); | |||
There was a problem hiding this comment.
This seems like quite a round-about way of doing implementing this feature, I mean, we have many steps to implement this feature:
- Click Reset link
- Send ajax requests to SubscriptionsController#index
- Return javascript to replace #subscriptions-form with the default subscriptions form html rendered in the javascript.
Why not just make the Reset link be a normal request to the controller? Also FYI reseting a user's subscriptions to the default can also be achieved by deleting all of the user's subscriptions. So it could be done with a destroy method on the controller i.e.
def destroy
subscriptions_form.reset_to_default
endAnd then in event_subscription/form.rb:
def reset_to_default
subscriber.event_subscriptions.delete_all
endThere was a problem hiding this comment.
@evanrolfe With the current approach a user can see the change and then decide if he wants it or not. Your proposal would directly apply the changes.
And regarding the roundtrip... we have that one way or the other. And without ajax we even have a full page reload.
evanrolfe
suggested changes
Jan 22, 2018
| @subscriptions_form = subscriptions_form | ||
| @subscriptions_form = subscriptions_form(default_form: params[:default_form]) | ||
|
|
||
| Rails.logger.debug "### ping# ##" |
PR#4336 added a switch that allows users to reset their notification subscription to default. This was hardcoded to clear all subscriptions. With this commit the default subscription settings are retrieved from the server via ajax and properly set in the ui. The user can then decide to save the new settings or cancel it by reloading the page.
bgeuken
force-pushed
the
update_subscriptions
branch
from
10471e1
to
cbd05fc
Compare
Codecov Report
@@ Coverage Diff @@
## master #4384 +/- ##
==========================================
- Coverage 88.13% 88.13% -0.01%
==========================================
Files 352 352
Lines 18691 18696 +5
==========================================
+ Hits 16474 16478 +4
- Misses 2217 2218 +1
|
evanrolfe
approved these changes
Jan 22, 2018
hennevogel
added a commit
to hennevogel/open-build-service
that referenced
this pull request
May 27, 2024
This got added in openSUSE#4186 and openSUSE#4384 and then removed while switching interfaces to bootstrap in openSUSE#8341. Fixes openSUSE#16198 which was caused by a spammer script going back and forth on routes with format=js because it could not acknowledge a status message anymore. Thanks spammer, keep up the good work!
hennevogel
added a commit
to hennevogel/open-build-service
that referenced
this pull request
May 28, 2024
This got added in openSUSE#4186 and openSUSE#4384 and then removed while switching interfaces to bootstrap in openSUSE#8341. Fixes openSUSE#16198 which was caused by a spammer script going back and forth on routes with format=js because it could not acknowledge a status message anymore. Thanks spammer, keep up the good work!
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.
#4336 added a switch that allows users to reset their notification
subscription to default. This was hardcoded to clear all subscriptions.
With this commit the default subscription settings are retrieved from
the server via ajax and properly set in the ui.
The user can then decide to save the new settings or cancel it by
reloading the page.