Skip to content

Fix don't require inactive authorization handlers#8122

Merged
leio10 merged 5 commits intodecidim:developfrom
mainio:fix/dont_require_inactive_authorization_handlers
Jul 21, 2021
Merged

Fix don't require inactive authorization handlers#8122
leio10 merged 5 commits intodecidim:developfrom
mainio:fix/dont_require_inactive_authorization_handlers

Conversation

@lahdeero
Copy link
Copy Markdown
Contributor

@lahdeero lahdeero commented Jun 9, 2021 •
edited
Loading

🎩 What? Why?

Currently when we change "Available authorizations" from system admin panel, it doesn't affect to component's permissions. So for example if we edit permissions from component, enable some authorization and after that we disable it from system admin, component still requires it.

Testing

  1. Basic decidim installation with seed data (e.g. decidim GitHub development_app)
  2. Go to /system
  3. Enable all available authorization options
  4. Go to /admin
  5. Configure both authorizations as required for a budgeting component
  6. Go to vote, you should see the authorization modal with two authorization options
  7. Go to /system
  8. Disable the other authorization from the organization
  9. Go to vote, you SHOULD see only the available authorization but you are seeing both options that you previously configured for the component

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

image

image

♥️ Thank you!

leio10
leio10 approved these changes Jun 14, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like @decidim/product to review this before merging it, as it's "subtle" behavior change.

Currently, the list of available authorization handlers managed through the system part is the list of authorizations that can be used for setting permissions. Disabling an authorization handler doesn't affect the components that are already configured.

With this PR, disabling an authorization handler will automatically stop using it everywhere.

I can think reasons for using both approaches, but I agree with @lahdeero that this new approach seems to be more intuitive.

@leio10 leio10 added module: core type: fix PRs that implement a fix for a bug labels Jun 14, 2021
@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Jul 21, 2021

LGTM, but I'd like @decidim/product to review this before merging it, as it's "subtle" behavior change.

I've discussed it with @carolromero and we both agree that in this case what we would expect is to NOT have the disabled authorization.

Go ahead with merging the PR, please. Thanks!

@leio10 leio10 merged commit 103ed6e into decidim:develop Jul 21, 2021
@ahukkanen ahukkanen deleted the fix/dont_require_inactive_authorization_handlers branch July 21, 2021 08:42
@leio10
Copy link
Copy Markdown
Contributor

leio10 commented Jul 21, 2021

Please @lahdeero, can you backport this PR to 0.24? Thanks!

ahukkanen pushed a commit to mainio/decidim that referenced this pull request Jul 21, 2021
@lahdeero @ahukkanen
Fix don't require inactive authorization handlers (decidim#8122)
9b42f2f
@ahukkanen ahukkanen mentioned this pull request Jul 21, 2021
Merged
12 tasks
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Jul 21, 2021

@leio10 Bacport is opened at #8223.

leio10 pushed a commit that referenced this pull request Jul 21, 2021
@ahukkanen @lahdeero
Fix don't require inactive authorization handlers (#8122) (#8223)
67532f0 
Co-authored-by: Eero Lahdenperä <eero.lahdenpera@gmail.com>
This was referenced Jul 21, 2021
Merged
Merged
entantoencuanto added a commit that referenced this pull request Jul 26, 2021
@entantoencuanto
Merge branch 'develop' into 7714-quilljs
e488132 
* develop: (32 commits)
  Remove obsolete rake webpack task (#8237)
  Active storage migrations service (#7902)
  Fix content type delegation to blank attachments (#8230)
  Evote bug fixing (#8220)
  Fix the proposal data migration for proposals without authors or organization (#8015)
  Bump addressable version because security issues (#8229)
  Online meetings iframe visibility with time (#8097)
  Meetings iframe and iframe URL (#8096)
  Remove flaky test on meetings (#8226)
  Fix broken tests after problematic PRs (#8224)
  Apply permissions system to comments (#8035)
  Set current_component as commentable when commentable is a participatory space (#8189)
  Fix don't require inactive authorization handlers (#8122)
  Improve metrics calculations performance (#8215)
  Fix performance issue in notification settings page (#8155)
  Active storage migration (#7598)
  Update manual installation guide in documentation (#8217)
  Load JS configuration in elections focus mode layout (#8213)
  Fix user activity pagination when there are hidden items (#8202)
  Make it possible to define SCSS settings overrides from modules (#8198)
  ...
roxanaopr pushed a commit to i-need-another-coffee/decidim that referenced this pull request Jul 29, 2021
@lahdeero @roxanaopr
Fix don't require inactive authorization handlers (decidim#8122)
f65ded4
roxanaopr pushed a commit to i-need-another-coffee/decidim that referenced this pull request Aug 10, 2021
@lahdeero @roxanaopr
Fix don't require inactive authorization handlers (decidim#8122)
b11d0f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@leio10 leio10 leio10 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

in-review module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lahdeero @andreslucena @leio10 @ahukkanen