Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to renew expired verifications (if renewable) #8192

Merged

Conversation

microstudi
Copy link
Contributor

@microstudi microstudi commented Jul 8, 2021

🎩 What? Why?

Currently, if a 2-steps (a verification with a custom engine) is renewable and has expired, the user is redirected to the action "new verification" which fails due the permission required is "to create" not "to renew".

This PR puts the renew path first regardless the expiration status if the verification is renewable.

image

♥️ Thank you!

@microstudi microstudi added the type: bug Issues that describe a bug label Jul 8, 2021
@andreslucena andreslucena changed the title allow to renew expired verifications (if renewable) Allow to renew expired verifications (if renewable) Jul 12, 2021
@andreslucena
Copy link
Member

Can you review the failing tests in CI @microstudi 🙏🏽 ? Thanks

@leio10 leio10 added type: fix PRs that implement a fix for a bug module: verifications and removed type: bug Issues that describe a bug labels Sep 1, 2021
@andreslucena
Copy link
Member

Also, I think we should also a spec to check for regressions.

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

As mentioned in the PR:

  1. there's a failing spec
  2. we should add a spec to catch regressions

@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. @carolromero & @andreslucena feel free to chime in.

@stale stale bot added the wontfix label Apr 18, 2022
@ahukkanen ahukkanen removed the wontfix label May 4, 2022
@ahukkanen
Copy link
Contributor

I have removed the wontfix label from this PR as @microstudi said today he is still planning to work on this issue.

@microstudi
Copy link
Contributor Author

@ahukkanen @andreslucena this should be ready now.

The failing spec was testing a wrong user journey (which passed the test before because the view was also wrong).
Now it tests the correct scenario (a verification renew) so regressions should be covered.

Sorry for taking so long to correct this.

Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Great!

I've tested this locally and it fixes the explained problem. I also remember I have faced the same issue myself in the past.


As a side note (not related to this PR):
I'm not quite sure what the renewable attribute does other than shows this modal before the renewal, since I could also re-authorize the another_dummy_authorization_handler after it had expired and renewable was set to false. The only difference was that it skipped this modal. But then again, this is a direct verification and not a 2-step.

I also understand this is something we can control from the authorization itself, so I believe this is intended behavior specific to the another_dummy_authorization_handler.

I also noticed that in the verifications module README.md it says currently the following:

Renewable authorizations. By default a participant can't renew its authorization

Then again we default the renewable attribute to true:

attribute :renewable, Boolean, default: true

But these are not related to this PR in particular. Just something I noticed when going through it.

@microstudi
Copy link
Contributor Author

You mean that README is wrong? or it should be the desired behavior and we should change the default? Not sure what this consequences can bring ...

@ahukkanen
Copy link
Contributor

You mean that README is wrong?

Yes, this is what I meant.

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽 Thanks for the fix!

@andreslucena andreslucena merged commit 2c05484 into decidim:develop May 9, 2022
andreslucena pushed a commit that referenced this pull request May 19, 2022
* allow to renew expired verifications (if renewable)

* fix test

* rubocop
andreslucena pushed a commit that referenced this pull request May 19, 2022
* allow to renew expired verifications (if renewable)

* fix test

* rubocop
andreslucena pushed a commit that referenced this pull request May 19, 2022
* allow to renew expired verifications (if renewable)

* fix test

* rubocop
andreslucena pushed a commit that referenced this pull request May 19, 2022
* allow to renew expired verifications (if renewable)

* fix test

* rubocop
ahukkanen pushed a commit that referenced this pull request May 24, 2022
* allow to renew expired verifications (if renewable)

* fix test

* rubocop

Co-authored-by: Ivan Vergés <ivan@platoniq.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: verifications type: fix PRs that implement a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants