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

fix: duplicate messages in recovery flow #2592

Merged
merged 2 commits into from
Jul 21, 2022
Merged

fix: duplicate messages in recovery flow #2592

merged 2 commits into from
Jul 21, 2022

Conversation

erolkskn
Copy link
Contributor

@erolkskn erolkskn commented Jul 15, 2022

Related issue(s)

#2512

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I am following the contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security. vulnerability,
    I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added or changed the documentation.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you. Unfortunately, several test suites are failing. Can you please address the failing tests? My hunch says that you can't just reset the messages there as it will cause some messages to be missing, and will need to use another approach.

@erolkskn
Copy link
Contributor Author

Thank you. Unfortunately, several test suites are failing. Can you please address the failing tests? My hunch says that you can't just reset the messages there as it will cause some messages to be missing, and will need to use another approach.

Thanks for your review, first error was about linter so i corrected it. But the problem with e2e tests doesn't seem to be caused by this commit, all the test suites related to recovery are actually passed, seems like there is a problem with oidc tests. Exact same errors happened after the commit aa6eb13. https://github.com/ory/kratos/runs/7325177589?check_suite_focus=true (which is parent commit of this commit). I can try to fix them up too, but i'm not sure whether i should create a new pr for it or continue with this one.

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #2592 (57f583f) into master (6c14b68) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2592   +/-   ##
=======================================
  Coverage   75.35%   75.35%           
=======================================
  Files         295      295           
  Lines       16355    16356    +1     
=======================================
+ Hits        12324    12325    +1     
  Misses       3092     3092           
  Partials      939      939           
Impacted Files Coverage Δ
selfservice/flow/recovery/error.go 78.84% <100.00%> (+0.41%) ⬆️

@aeneasr
Copy link
Member

aeneasr commented Jul 16, 2022

I reran the suite to see if it is a flaky test, but apparently they are still failing. All other branches are passing though. I do think it has to do something with the changes in this PR!

Regarding the change itself, could you please add a test case that ensures that we don't regress with this in the future? Imagine someone else moving this line of code because they don't understand it's there -> the functionality is broken again because we have no tests.

Thank you :)

@erolkskn
Copy link
Contributor Author

I didn't mean that "tests are flaky" but rather i think tests are failing due to the commit aa6eb13. Your commit from 5 hours ago had the exact same problem in e2e tests . https://github.com/ory/kratos/runs/7369485507?check_suite_focus=true d1b6b40. Ofc i can add tests, but no matter what i will do tests won't run successfully because test cases doesn't fail because of this commit. I prepared some screenshots in order to explain it better. Here are the screenshots of pipeline logs of commit aa6eb13 (i strongly believe that tests are failing due to that commit), your commit from 5 hours ago d1b6b40, and this one. They are all failing in the exact same way 5 of OIDC Suite Cases are failing

Pipeline logs of commit aa6eb13.

first

Pipeline logs of commit d1b6b40. That's the one you committed 5 hours ago

aenasr

Pipeline logs of this commit

this

@erolkskn erolkskn requested a review from aeneasr July 19, 2022 13:36
@aeneasr
Copy link
Member

aeneasr commented Jul 19, 2022

Awesome, thanks! I think this looks much better now :) I rebased on master which hopefully resolves some of the flaky tests :)

aeneasr
aeneasr previously approved these changes Jul 19, 2022
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! 🎉 Your contribution makes Ory better :)

@aeneasr
Copy link
Member

aeneasr commented Jul 19, 2022

Code LGTM, let's wait for CI :)

@aeneasr aeneasr merged commit 43fcc51 into ory:master Jul 21, 2022
@vinckr
Copy link
Member

vinckr commented Jul 26, 2022

Hello @erolkskn
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

@erolkskn
Copy link
Contributor Author

Hello @erolkskn Congrats on merging your first PR in Ory tada ! Your contribution will soon be helping secure millions of identities around the globe earth_asia. As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community. Please drop me an email and I will forward you the form to claim your Ory swag!

Oh what a nice gesture, thank you guys for that 🙂

@Benehiko Benehiko mentioned this pull request Aug 2, 2022
6 tasks
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants