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

Use PasswordResetView and PasswordResetConfirmView #1135

Merged
merged 7 commits into from
Aug 20, 2021

Conversation

chris34
Copy link
Member

@chris34 chris34 commented Aug 23, 2020

warnings were

/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/django/utils/decorators.py:149: RemovedInDjango21Warning: The password_reset() view is superseded by the class-based PasswordResetView().
  response = view_func(request, *args, **kwargs)
/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/django/contrib/auth/views.py:54: RemovedInDjango21Warning: The password_reset_confirm() view is superseded by the class-based PasswordResetConfirmView().
  return func(*args, **kwargs)

see https://docs.djangoproject.com/en/3.0/releases/1.11/#django-contrib-auth

  • The tests run and also seem to basically check, whether a reset email was send. Nevertheless, i could not see any mail on the command line while testing it manually. Now works

@chris34 chris34 self-assigned this Aug 23, 2020
@chris34 chris34 changed the base branch from testing to staging November 2, 2020 01:00
@chris34 chris34 force-pushed the reset_views branch 2 times, most recently from dd901ca to 0be295e Compare December 12, 2020 23:15
@chris34 chris34 force-pushed the reset_views branch 2 times, most recently from ffecf9b to 3491674 Compare December 19, 2020 00:24
@chris34 chris34 changed the title WIP: Use PasswordResetView and PasswordResetConfirmView Use PasswordResetView and PasswordResetConfirmView Dec 19, 2020
@chris34 chris34 requested review from Lyra2108 and v-gar December 19, 2020 00:29
@chris34
Copy link
Member Author

chris34 commented Dec 19, 2020

I tried to get the cause why a BDD test for the last-password-view now fail and if have no clue. We have classic django unit-/integration-tests for the same view, so skipping the BDD test is IMO ok.

Interestingly, the other BDD test that did not run anymore, does now run after a selenium update. Hope that is not too confusing to put it all in one PR. :D

})

return (
u for u in possible_users
Copy link
Member

Choose a reason for hiding this comment

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

Minor: would you mind some more letters then "u" ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the naming from upstream → https://github.com/django/django/blob/stable/1.11.x/django/contrib/auth/forms.py#L273

I would try to have as small changes to upstream as possible. (There was already a CVE in this code…) However, i see no ways to avoid c&p this method-code, as (like mentioned in the docstring) we have no is_active DB-field.

@@ -13,6 +13,7 @@ Feature: Password forgotten
| item |
| id_email |

@skip
Copy link
Member

Choose a reason for hiding this comment

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

is this unfixable ?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO yes. See my comment and the commit-message for more details.

It could be also caused by using too much connections (the problem showed in the Django 2.2 PR), but i dont know.

Seems like the other unskipped test does also not run any more, i have no other idea than skipping it again… If i test it manually, all seems fine.

chris34 added 7 commits July 31, 2021 00:03
warnings were

```
/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/django/utils/decorators.py:149: RemovedInDjango21Warning: The password_reset() view is superseded by the class-based PasswordResetView().
  response = view_func(request, *args, **kwargs)
/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/django/contrib/auth/views.py:54: RemovedInDjango21Warning: The password_reset_confirm() view is superseded by the class-based PasswordResetConfirmView().
  return func(*args, **kwargs)
```

see https://docs.djangoproject.com/en/3.0/releases/1.11/#django-contrib-auth
This is needed as `is_active` is no attribute in the DB model.
Futhermore, the form is now more similar to upstream.
This scenario does not run currently. The browser seem to just displays
a blank page after submitting the form (html is `<html><head></head><body></body></html>`).

However, manually tests are just fine as expected. The test runs
fine if run alone. This is also the case, if only
tests marked as skipped are run
(command `DJANGO_SETTINGS_MODULE='tests.bdd.settings.general' coverage run --rcfile tests/bdd/settings/.coveragerc -m behave --tags=skip`).
So there seems to be some™ global state somewhere™, but I was not able
to locate it…

The following did not help:

 * wating some seconds via `context.browser.implicitly_wait` or
   `WebDriverWait`
 * add `context.browser.refresh` before `context.browser.quit`

traceback was f.e.

```
  Scenario: The user requests an new password to his mail  # tests/bdd/password_forgotten.feature:16
    Given I am on the "lost_password" page                 # tests/bdd/steps/navigation.py:6 0.411s
    When I fill out the form                               # tests/bdd/steps/form.py:4 0.253s
      | field    | value                |
      | id_email | test_mail@invalid.de |
    Then it should be successful                           # tests/bdd/steps/assertions.py:28 0.044s
      Traceback (most recent call last):
        File "/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
        File "/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "tests/bdd/steps/assertions.py", line 31, in step_impl
          assert context.browser.find_element_by_css_selector('.message.success')
        File "/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/selenium/webdriver/remote/webdriver.py", line 598, in find_element_by_css_selector
          return self.find_element(by=By.CSS_SELECTOR, value=css_selector)
        File "/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/selenium/webdriver/remote/webdriver.py", line 976, in find_element
          return self.execute(Command.FIND_ELEMENT, {
        File "/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/selenium/webdriver/remote/webdriver.py", line 321, in execute
          self.error_handler.check_response(response)
        File "/home/chris/.venvs/inyoka3/lib/python3.8/site-packages/selenium/webdriver/remote/errorhandler.py", line 242, in check_response
          raise exception_class(message, screen, stacktrace)
      selenium.common.exceptions.NoSuchElementException: Message: no such element: Unable to locate element: {"method":"css selector","selector":".message.success"}
        (Session info: headless chrome=87.0.4280.66)

      Captured stdout:
      <html><head></head><body></body></html>
```
@chris34
Copy link
Member Author

chris34 commented Aug 20, 2021

As discussed in IRC, i'll merge this as is, because it seems that nobody wants to review it again.

@chris34 chris34 merged commit 4c3528d into inyokaproject:staging Aug 20, 2021
@chris34 chris34 deleted the reset_views branch August 20, 2021 19:23
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.

2 participants