AJAX authorization modals by leio10 · Pull Request #3753 · decidim/decidim

leio10 · 2018-07-02T12:05:37Z

🎩 What? Why?

Currently, authorization modals are rendered in every page of a component, checking in advance the authorization status of the user for each handler related to every authorizable action defined by the component. Then, they can be easily shown when the user tries to perform an unauthorized action. This approach is simple and very performant, but also has drawbacks.

The main drawback of this approach is that it limits the ability to add action buttons outside the component pages, since it would require to render a lot of authorizations modals for a lot of components or write a very complex code that smartly detects which modals are needed on each page. Using cards we can render information of any resource on a participatory space home page or even in the main home page, but we can't add a "Call to action" button, since we are not rendering all its related authorization modals. Instead, we are forced to add a link to a page handled by the component engine, where we can render a button that can use an authorization modal if needed.

This PR changes this behavior, loading the authorization modal contents using AJAX requests when the user tries to perform an unauthorized action. This way, the modal displaying is a little bit slower, but it can dynamically check the authorization status of the user for that action, so it can be used anywhere.

To be more specific, the only requirement to use the action_authorized_link_to and action_authorized_button_to helpers is to have the current_component defined, which is currently done in the components base controller and in some cells.

📌 Related Issues

📋 Subtasks

Add a "Loading" message until the modal contents are loaded
Fix/complete tests
Recover cache of authorization result
Add CHANGELOG entry

📷 Screenshots (optional)

leio10 · 2018-07-02T13:37:28Z

@mrcasals @josepjaume @oriolgual I don't know if you have already found the drawback that I comment on the description (for example, with proposals), or even if it is done deliberately to avoid users to perform actions before visiting the resource page. In my case, I'm facing this issue when trying to render some cells on the home page; I don't know if that kind of scenario is on the project roadmap.

Also, I've had problems with naming. I couldn't implement the new controller in the verifications gem because its needed to be used from the core gem. On the other hand, I couldn't give the name authorizations to the new controller because that route is used by the verifications gem. I've finally choosed the modals name for the controller, but I'm not happy with it, since future AJAX modal requests shouldn't be handled by this controller. Any name suggestion would be very appreciated 😃

deivid-rodriguez

@leio10 Looks great to me and I don't think the slight delay when loading modals is an issue, although we might want to add some "loading icon tweaks" to the buttons or something to prevent unresponsiveness.

I added some minor comments but in general totally in favor of doing this!

deivid-rodriguez · 2018-07-04T12:22:44Z

decidim-consultations/app/views/decidim/consultations/questions/_vote_button.html.erb

-                                      data: { disable: true },
-                                      class: "card__button button expanded success disabled",
-                                      id: "unvote_button" do %>
+      <%= button_to decidim_consultations.question_question_votes_path(question),


Why was this changed? The interface should be the same and the only change would be that the modal is ajaxly loaded, right?

Yes, you are right that is not strictly related to this change. Is the same fix than this one but in the consultations module. Both consultations and initiatives are not components, but participatory spaces, so they don't have authorizations. I understand that these helpers are only used to show the login modal if needed.

For these cases, I defined the logged_button_to and logged_link_to and used them in this file and in the initiatives module templates.

But in this very specific case, this is not needed, because it is only rendered if the user is signed in. I guess that is the same reason why @mrcasals has done this.

Aha, understood! 👍

deivid-rodriguez · 2018-07-04T12:35:13Z

decidim-core/app/controllers/decidim/modals_controller.rb

+  class ModalsController < Decidim::ApplicationController
+    helper_method :status, :authorize_action_path
+
+    def authorization


Regarding the naming issue you mentioned, what about changing the controller name to AuthorizationModalsController and this action to show?

deivid-rodriguez · 2018-07-04T12:37:12Z

decidim-core/config/locales/ca.yml

        close_modal: Tanca el modal
        filter: Filtre
        filter_by: Filtra per
-        unfold: Desplegar


I don't think this key movement is intended.

Thanks! I catched it in the spanish translation but missed it here.

deivid-rodriguez · 2018-07-04T12:42:23Z

decidim-core/spec/helpers/decidim/action_authorization_helper_spec.rb

+
+        it "renders with a block" do
+          rendered = helper.action_authorized_link_to("foo", "fake_path") { "Link" }
+


Super tiny style note, you might want to remove this blank line for consistency with the other specs (although I think visually splitting non assertions from assertions reads better, so you might as well add a blank line to the other specs).

I agree with you that it reads better with an extra space there, and I don't see a clear guideline in other spec files, so I'm adding blank lines to every other specs

I like having non-assertions and assertions separate, but It's a rule I don't enforce even on myself so... 🤷‍♂️ There's some inconsistency throughout the code about this.

Yeah, since rubocop-rspec don't support it, it's hard to enforce. Maybe I'll do a feature request :)

leio10 · 2018-07-04T13:13:28Z

@deivid-rodriguez Thanks for the feedback!! ❤️ ❤️

I have "Adding a Loading message" in the pending tasks list, but I didn't see any loading message in the rest of the project, so I don't have a place to copy from. 😞

deivid-rodriguez · 2018-07-04T13:21:21Z

Yeah, I don't recall any loading spinners in the project either... And it doesn't look like foundation reveal has anything built-in. Maybe this thread has some ideas: https://foundation.zurb.com/forum/posts/35569-reveal-modal-loading-state.

mrcasals · 2018-07-04T13:26:11Z

@leio10 the only "loading" message we have is on the processes list, when you change the filters (below the highlighted processes). I think a spinner would be useful in other places too (eg when filtering proposals list)

leio10 · 2018-07-04T13:32:37Z

@deivid-rodriguez @mrcasals maybe we could just take an icon from https://decidim-design.herokuapp.com/public/library/pattern-library and make it spin with CSS. I guess that loop-circular, sun, reload or apperture should work. What do you think?

leio10 · 2018-07-04T14:31:18Z

@deivid-rodriguez I think I've addressed all your suggestions
@mrcasals I've added a CSS spinner borrowed from http://jedwatson.github.io/react-select/ that IMHO looks very nice, let me know if there is something you would change

deivid-rodriguez · 2018-07-04T15:17:19Z

@leio10 Just tried it. It looks awesome! Screenshot below (don't worry, I added sleep 2 in the controller, it's not that slow 😄. As a matter of fact, on my computer the spinner is almost invisible by default).

deivid-rodriguez · 2018-07-04T15:21:25Z

decidim-core/app/controllers/decidim/authorization_modals_controller.rb

+    layout false
+
+    def show
+      render


I don't think you need the render as long as there's an unlerlying template with the right name. I actually learnt from @oriolgual that you don't even need the action at all, as long as the template exists with the right name, although I find that style very obscure...

leio10 · 2018-07-04T16:05:59Z

I hope I'm finished this PR with these last commits.

I've updated the CHANGELOG file and I've recovered the ActionAuthorization concern to cache authorizations results, but with some differences:

I've used a request.env entry to ensure that unique authorization check is done once in each request (previously, each cell was having its own cache, so the proposals page perform the same check for each proposal). @mrcasals @deivid-rodriguez Are you OK with this use of that variable?
I'm not including this concern into the components base controller, but in the application controller, in order to allow every page to use it (this is the main reason to use AJAX modals instead of prerendered modals).
I'm including the component id in the cache key, to correctly cache results for actions with the same name but from different component.

mrcasals · 2018-07-05T07:19:58Z

Using cards we can render information of any resource on a participatory space home page or even in the main home page, but we can't add a "Call to action" button, since we are not rendering all its related authorization modals. Instead, we are forced to add a link to a page handled by the component engine, where we can render a button that can use an authorization modal if needed.

@leio10 when adding cells, we weren't sure you should be able to vote a proposal from the search page, so we didn't implement it and didn't check if that worked 😅

mrcasals · 2018-07-05T07:36:29Z

@leio10 code looks really good, and the authorizations sections it's something mostly you and @deivid-rodriguez have been working on, so if you two don't see any possible error on it then I'm totally 👍 on the change.

About the request.env I don't see a problem on this, but let's wait for @deivid-rodriguez's comments before merging it 😄

leio10 · 2018-07-05T07:58:56Z

@leio10 when adding cells, we weren't sure you should be able to vote a proposal from the search page, so we didn't implement it and didn't check if that worked 😅

@mrcasals I'm sorry if that seemed like a complain about the code, it was just an argument to justify the need of this PR. I'm very glad with the use of cells, and having that limitation was a little "thorn in the side" (or "stone in the shoe", as we would say in spanish). 😄

mrcasals · 2018-07-05T08:02:17Z

I'm sorry if that seemed like a complain about the code, it was just an argument to justify the need of this PR

I didn't take it as a complaint, I just wanted to explain why this isn't currently possible 😄

deivid-rodriguez · 2018-07-05T12:01:16Z

Actually, Rails released this feature for this exact use case, no?

deivid-rodriguez · 2018-07-05T12:23:50Z

Brain storming a bit here. What if we pass the action authorization cache as cell context in the base card? I think we're doing the same thing with the current_user.

leio10 · 2018-07-05T12:53:42Z

Actually, Rails released this feature for this exact use case, no?

I like that feature, but I think is also more for the current_... variables. 😄
If we start using it for those variables, we could think on adding an action_authorization_cache variable, or event a generic request_cache variable .

Brain storming a bit here. What if we pass the action authorization cache as cell context in the base card? I think we're doing the same thing with the current_user.

I like this approach, I think is the best option for this moment 👍

leio10 · 2018-07-05T15:10:40Z

@deivid-rodriguez I've been playing for a while with cells context and things get much more complicated (the ActionAuthorization concern has to be different for controllers and cells because cells get the cache reference from the context, it fails when there are cells rendered inside cells, etc.). 😅

I would leave it as is, and would evaluate in the future the convenience of having one or more CurrentAttributes classes to store the variables stored in the request.env hash. @deivid-rodriguez @mrcasals What do you think?

deivid-rodriguez · 2018-07-05T15:16:10Z

Sounds good to me @leio10! 👍

mrcasals · 2018-07-09T07:11:38Z

Re: Current objects, I remember some discussion on this and we (@decidim/lot-core) didn't like the abstraction, but I can't find the issue/PR now...

Apart from that, everything looks good to me. @deivid-rodriguez do you mind reviewing the code? I trust you more than myself here 😄

leio10 · 2018-07-09T07:20:16Z

@mrcasals the code doesn't changed since the last @deivid-rodriguez revision, I just rebased it to resolve conflicts and force to report coverage. 😄

mrcasals · 2018-07-09T07:25:55Z

@leio10 nice! So merging it! 😄

leio10 · 2018-07-09T07:28:34Z

🎉 🎉 Hooray! Thanks @mrcasals!

mrcasals · 2018-07-09T07:48:36Z

@leio10 thank you for your contributions to the project! 😄

ghost assigned leio10 Jul 2, 2018

ghost added the status: WIP label Jul 2, 2018

leio10 force-pushed the enhancement/ajax_authorization_modals branch 5 times, most recently from 155cf76 to d30198e Compare July 3, 2018 14:05

leio10 added the status: needs feedback label Jul 4, 2018

deivid-rodriguez reviewed Jul 4, 2018

View reviewed changes

leio10 force-pushed the enhancement/ajax_authorization_modals branch 3 times, most recently from 21ef12f to 70976c9 Compare July 4, 2018 14:30

deivid-rodriguez reviewed Jul 4, 2018

View reviewed changes

leio10 force-pushed the enhancement/ajax_authorization_modals branch 2 times, most recently from eb41d7d to 6e8da74 Compare July 4, 2018 16:03

mrcasals added in-review and removed status: WIP labels Jul 5, 2018

mrcasals changed the title ~~WIP: AJAX authorization modals~~ AJAX authorization modals Jul 5, 2018

ghost added the status: WIP label Jul 5, 2018

leio10 removed the status: WIP label Jul 5, 2018

deivid-rodriguez previously approved these changes Jul 5, 2018

View reviewed changes

leio10 added 9 commits July 6, 2018 18:35

Added support to dynamically load modal contents

4ac09f1

Authorization modals converted to AJAX modals

946d89c

Removed ActionAuthorization hack from consultations.

bc9caa6

Fixed bug added when removing ActionAuthorization hack from initiatives

9422476

Be prepared to show the authorization modals in any page.

ca70496

Tests and lint fixes

b76ed33

Better controller name, minor i18n fix and loading spinner added

783d1cc

Cache authentication results for the same component and action.

dc33f45

Changelog entry updated

3b19f49

leio10 dismissed deivid-rodriguez’s stale review via 3b19f49 July 6, 2018 16:37

leio10 force-pushed the enhancement/ajax_authorization_modals branch from 726b170 to 3b19f49 Compare July 6, 2018 16:37

ghost added the status: WIP label Jul 6, 2018

leio10 removed the status: WIP label Jul 7, 2018

mrcasals approved these changes Jul 9, 2018

View reviewed changes

mrcasals merged commit dff536b into decidim:master Jul 9, 2018

leio10 deleted the enhancement/ajax_authorization_modals branch July 9, 2018 07:28

leio10 mentioned this pull request Jul 9, 2018

Action authorizations at resource levels #3804

Merged

3 tasks


		it "renders with a block" do
		rendered = helper.action_authorized_link_to("foo", "fake_path") { "Link" }

Uh oh!

Conversation

leio10 commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

leio10 commented Jul 2, 2018

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leio10 commented Jul 4, 2018

Uh oh!

deivid-rodriguez commented Jul 4, 2018

Uh oh!

mrcasals commented Jul 4, 2018

Uh oh!

leio10 commented Jul 4, 2018

Uh oh!

leio10 commented Jul 4, 2018

Uh oh!

deivid-rodriguez commented Jul 4, 2018

Uh oh!

deivid-rodriguez Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leio10 commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrcasals commented Jul 5, 2018

Uh oh!

mrcasals commented Jul 5, 2018

Uh oh!

leio10 commented Jul 5, 2018

Uh oh!

mrcasals commented Jul 5, 2018

Uh oh!

deivid-rodriguez commented Jul 5, 2018

Uh oh!

deivid-rodriguez commented Jul 5, 2018

Uh oh!

leio10 commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leio10 commented Jul 5, 2018

Uh oh!

deivid-rodriguez commented Jul 5, 2018

Uh oh!

mrcasals commented Jul 9, 2018

Uh oh!

leio10 commented Jul 9, 2018

Uh oh!

mrcasals commented Jul 9, 2018

Uh oh!

leio10 commented Jul 2, 2018 •

edited

Loading

deivid-rodriguez Jul 4, 2018 •

edited

Loading

leio10 commented Jul 4, 2018 •

edited

Loading

leio10 commented Jul 5, 2018 •

edited

Loading