Codecov Report

Merging #2024 into master will increase coverage by 0.02%.
The diff coverage is 98.78%.

@@            Coverage Diff            @@
##           master   #2024      +/-   ##
=========================================
+ Coverage   98.58%   98.6%   +0.02%     
=========================================
  Files        1190    1239      +49     
  Lines       27442   28439     +997     
=========================================
+ Hits        27053   28042     +989     
- Misses        389     397       +8

Hei! This should be ready now for an initial review. I finally added "postal letter verifications" as well in here. Let me know if you want it splitted away, or if it's fine.

I've updated the PR's description and I'll be posting some more screenshots later today.

Hi @deivid-rodriguez - this looks very promising. Before reviewing the code though, there's a tricky scenario you maybe didn't have into account: managed users. Managed users can't be impersonated without re-entering the user's authorization data, which looks kind of incompatible with "deferred" (engine?) authorizations.

Maybe we could include that information (deferred authorizations) in the manifest in order to exclude them from managed users? It doesn't seem trivial to manage it in a different way.

Thanks!

Hi @deivid-rodriguez - this looks very promising. Before reviewing the code though, there's a tricky scenario you maybe didn't have into account: managed users. Managed users can't be impersonated without re-entering the user's authorization data, which looks kind of incompatible with "deferred" (engine?) authorizations.

Yes, I did! 😄 See #1871 (comment). My solution is to only use direct authorization handlers for impersonations.

Maybe we could include that information (deferred authorizations) in the manifest in order to exclude them from managed users? It doesn't seem trivial to manage it in a different way.

Regarding the implementation, at the moment authorization handlers are not registered through a manifest but just like they are registered now through a global configuration. So to figure out the "authorization" type I "auto-detect" it:

• If registered through manifest, it's a workflow (deferred) method.
• If constantizable to a class, then it's a handler (direct) method.

It's this magic here:

I wanted to keep backwards compatibility in this first MVP and it seemed nice to still be able to register handlers directly as it's done now. But looking backwards I should've probably unified everything under a manifest.

Some heads up here, I renewed the screenshots. While recording them, I found several bugs and improvements which I've fixed and added. There's some general fixes in here that I could extract out of this PR if you wanted me to.

@deivid-rodriguez screenshots look superawesome! I'll try to find some time today to review the PR! 😄

@mrcasals Do you see them as blurry as I do? The screen recording process I'm using not only quasi-freezes my laptop but also doesn't generate great gifs. But things should look better in real life 😃

Ah yes, GIFs look blurry, but that's a problem with GIFs I guess, so we can't do much about it...

@deivid-rodriguez with this PR, does this mean we can extend decidim with new authorizations bundled in a gem? I'm thinking about the Spanish IDs (DNI, NIE, passport) we're using in Barcelona/Terrassa/Sant Cugat. They are mostly the same (census authorization), they mostly use the same fields, and only change how to check the values to an API.

Would we be able to extract this in a gem?

@deivid-rodriguez über-nice work! I think I'll need to work on adding a new authorization method in order to fully understand how to do that, because it looks a bit complicated, but that's OK.

Should we move the authorizations we have in core to this new engine? They might be simple enough, but this way we keep similar things together.

Apart from that, really nice work, thanks! ❤️

@deivid-rodriguez über-nice work! I think I'll need to work on adding a new authorization method in order to fully understand how to do that, because it looks a bit complicated, but that's OK.

Yes, it is a bit complicated... In a future iteration I think we could unify everything under a manifest like @josepjaume intended / suggested. This will be simpler implementation-wise and towards users since we'll have a single way of registering (and documenting) authorization methods.

Something like:

 Decidim::Verifications.register_verification_method(:id_documents) do |verification| 
   verification.engine = Decidim::Verifications::IdDocuments::Engine 
   verification.admin_engine = Decidim::Verifications::IdDocuments::AdminEngine 
 end

or, for current handlers,

 Decidim::Verifications.register_verification_method(:barcelona_census) do |verification| 
   verification.form = Decidim::Barcelona::CensusHandler 
 end

However, I think we can release this way for now, since it allows us to start experimenting with this new authorization methods without breaking compatibility with the old ones (and thus not having to deal yet with data migrations, and so on).

Should we move the authorizations we have in core to this new engine? They might be simple enough, but this way we keep similar things together.

Absolutely! I scheduled myself to do this, but I forgot! Assuming you mean under the new "gem" (note that there's one "engine" per authorization method, or even two if the authorization method needs admin intervention).