Allow AuthorizationHandlers to customize authorization status check process by leio10 · Pull Request #2438 · decidim/decidim

leio10 · 2018-01-04T18:14:26Z

🎩 What? Why?

This change is the second part of #2435, explained in #2314. It adds a DefaultActionAuthorizer class with public methods that could be overriden by child classes. The class to be used for each handler is defined in the handler's WorkflowManifest.

Most of the logic of the status_data method of the ActionAuthorizer class was moved to the authorize method of the new class.

More than the half of the effort of this PR was to allow authorization handler to inform the user about the implemented logic, this include:

Allow to define new status codes, with all it related translations. Authorization modals view was refactored to implement this.
Show additional messages to the user. This change is also included in authorization modals view changes.
Allow forms to change behavior based on the authorization options. These options are added to query string when redirecting the user to the form.

A future PR should allow authorization handlers to define forms for options setting and avoid admin users to learn JSON.

📌 Related Issues

Related to Short-term authorizations #2314
Related to AuthorizationHandler initialization moved outside of the AuthorizationStatus class #2435

📋 Subtasks

Add hooks
Add an example of use
Complete tests
Update docs and changelog

📷 Screenshots (optional)

👻 GIF

codecov · 2018-01-04T18:48:15Z

Codecov Report

Merging #2438 into master will increase coverage by <.01%.
The diff coverage is 99.31%.

@@            Coverage Diff             @@
##           master    #2438      +/-   ##
==========================================
+ Coverage   98.68%   98.69%   +<.01%     
==========================================
  Files        1296     1297       +1     
  Lines       30363    30354       -9     
==========================================
- Hits        29964    29958       -6     
+ Misses        399      396       -3

josepjaume · 2018-01-08T07:52:38Z

decidim-verifications/lib/decidim/verifications/hooks.rb

+
+module Decidim
+  module Verifications
+    class Hooks


This should probably be called DefaultHooks, as it comes with a default implementation.

Also, not sure about hook as a name for this abstraction.

What about calling it DefaultAuthorizationHandlerAuthorizer? it is a little long, but I think is the most accurate name, since it provides the logic for ActionAuthorizer that is inside the specific authorization handler. I would also change all references to hooks with authorizer.

What about using DefaultActionAuthorizer and action_authorizer as a method name?

Ok, I will prepare that change. 👍

oriolgual · 2018-01-08T09:31:28Z

decidim-verifications/README.md

+These are particulary useful when used within verification options, which are
+set in the admin zone related to a feature action. As a result, a verification
+method will be allowed to change the authorization logic and the appearance based
+on the context where the authorization is being performed.


Maybe mention an example so a developer or a user can understand its value?

And also an example on how to set it at the admin zone (like the screenshot)

What do you think about this? I'm not very good in english developer-human communications, any help would be very appreciated. 😅

oriolgual

Nice work!

oriolgual

Works for me!

mrcasals · 2018-01-12T08:07:33Z

@leio10 is this ready for reviews?

… as a sample of hooks use.

…omments.

leio10 · 2018-01-12T08:59:15Z

@mrcasals it should be ready to merge now.
The root of the conflicts is the changelog file, because I'm trying to fix a typo in the markdown used to refer to the related PR in the last six entries: we are using [\1234] instead of [\#1234]. Everyone is copying the format from last entry in the CHANGELOG, so everyone is making the same mistake 🙈. So, it will be mergeable until the next merge to the master, when conflicts will appear again.

mrcasals · 2018-01-12T09:04:17Z

@leio10 LOL thanks!! I'm gonna review it and hopefully we can merge this without any other conflicts!

leio10 · 2018-01-12T09:56:53Z

@mrcasals It seems that codecov is stuck with this PR 😢

mrcasals · 2018-01-12T10:04:26Z

@leio10 shhhhhhhhh 🤐

mrcasals · 2018-01-12T10:05:24Z

Merged! 🎉

About CodeCov, it's not the first time it gets stuck, and in some other PRs we've skipped the checks and merged the Pr anyway.

leio10 force-pushed the enhancement/improved-authorizations-step2 branch from 9f2f426 to 8c9b635 Compare January 4, 2018 18:48

leio10 force-pushed the enhancement/improved-authorizations-step2 branch 6 times, most recently from 1e92d7c to 464d13e Compare January 5, 2018 16:47

josepjaume reviewed Jan 8, 2018

View reviewed changes

oriolgual reviewed Jan 8, 2018

View reviewed changes

oriolgual previously approved these changes Jan 8, 2018

View reviewed changes

leio10 dismissed oriolgual’s stale review via 1925943 January 8, 2018 10:36

oriolgual previously approved these changes Jan 8, 2018

View reviewed changes

leio10 dismissed oriolgual’s stale review via 3a0f03e January 8, 2018 12:43

leio10 force-pushed the enhancement/improved-authorizations-step2 branch from 3a0f03e to 689c6aa Compare January 8, 2018 15:03

oriolgual previously approved these changes Jan 9, 2018

View reviewed changes

mrcasals dismissed oriolgual’s stale review via e76e4cc January 9, 2018 10:25

ghost assigned mrcasals Jan 9, 2018

ghost added the in-progress label Jan 9, 2018

leio10 force-pushed the enhancement/improved-authorizations-step2 branch 2 times, most recently from 4dca389 to ddca15c Compare January 10, 2018 13:32

mrcasals removed their assignment Jan 12, 2018

leio10 added 7 commits January 12, 2018 09:48

Added hooks to decidim-verifications.

30ebd1a

Using hooks in DummyAuthorizationHandler for postal code restriction,…

152f8fe

… as a sample of hooks use.

Removed Wisper::Publisher from ActionAuthorizer.

f60af2d

Tests fixed.

c1e6d2f

More tests fixed and some added.

94daed2

Documentation added and changelog updated.

774847d

Renamed Hooks class to DefaultActionAuthorizer and adapted docs and c…

4d4aadb

…omments.

Added an example of use of custom ActionAuthorizer classes.

c972392

leio10 force-pushed the enhancement/improved-authorizations-step2 branch from ddca15c to c972392 Compare January 12, 2018 08:51

mrcasals approved these changes Jan 12, 2018

View reviewed changes

oriolgual approved these changes Jan 12, 2018

View reviewed changes

mrcasals merged commit dc04987 into decidim:master Jan 12, 2018

ghost removed the in-progress label Jan 12, 2018

leio10 mentioned this pull request Jan 12, 2018

Recover lose part of an entry in the changelog. #2480

Merged

deivid-rodriguez mentioned this pull request Feb 26, 2018

Not force admins to learn JSON #2822

Closed

juliesimon mentioned this pull request Mar 27, 2018

Understanding (and fixing ?) Managed users OpenSourcePolitics/decidim#66

Closed

3 tasks

Xfolchf mentioned this pull request Apr 9, 2018

Two buttons "create" and "send" when creating a managed user #3161

Closed

leio10 mentioned this pull request Jul 10, 2018

Action authorizations at resource levels #3804

Merged

3 tasks

Uh oh!

Conversation

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

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Uh oh!

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

Uh oh!

Codecov Report

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!

oriolgual left a comment

Choose a reason for hiding this comment

Uh oh!

oriolgual left a comment

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Jan 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

mrcasals commented Jan 12, 2018

Uh oh!

leio10 commented Jan 12, 2018

Uh oh!

mrcasals commented Jan 12, 2018

Uh oh!

mrcasals commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leio10 commented Jan 4, 2018 •

edited

Loading

codecov bot commented Jan 4, 2018 •

edited

Loading

leio10 commented Jan 12, 2018 •

edited

Loading