Please Apply #5801 - GET parameters are ignored in redirect for Admin
Rozza
This is a bump for ticket #5801 it was marked as accepted back in
February but has not yet been applied!
Apologies if this is against protocol - it just seems this simple
patch has been lost amongst the many!
Ross
Russell Keith-Magee
If you look at the triage process [1], you will see that there is a
good reason that the patch hasn't been applied. Accepted means that we
acknowledge that the bug is real, not that the patch is ready. A patch
will only be applied when the ticket has been marked ready for
checkin.
The obvious reason for this ticket not being ready for checkin is that
the patch doesn't have any tests. This should be a relatively simple
case to check with the built in test client, so there is no excuse for
not having tests. Write some tests, and we'll get this ticket
committed.
[1] http://www.djangoproject.com/documentation/contributing/#ticket-triage
Yours,
Russ Magee %-)
Rozza
As there aren't any tests for the staff_member_required or even
request.get_full_path() I wasn't 100% sure what the procedure would be
as these are used through out Django.
As it's such a minor change does it warrant the bureaucracy of
requiring tests to have it implemented?
I think definitely there should be a ticket opened to ensure that
tests are written to test the logic staff_member_required and
request.get_full_path(). At the moment I'm not sure where they would
fit.
Happy to help either way.
Ross
On Jul 18, 9:04 am, "Russell Keith-Magee" <freakboy3...@gmail.com>
wrote:
Russell Keith-Magee
>
> Ah no problems!
>
> As there aren't any tests for the staff_member_required or even
> request.get_full_path() I wasn't 100% sure what the procedure would be
> as these are used through out Django.
>
> As it's such a minor change does it warrant the bureaucracy of
> requiring tests to have it implemented?
Yes. Next question? :-)
There are some parts of Django that either do not have tests. These
usually correspond to the oldest parts of the framework, which existed
before Django had a full testing framework. Tha'ts not an excuse to
avoid writing tests - it just means that your tests will be the first
tests in that area.
The tests serve multiple purposes:
- they demonstrate the existence of a problem, which makes the triage
process easier
- they demonstrate that your patch fixes the problem
- they prevent the problem from happening again in the future.
With very few exceptions, nothing gets committed to the code base
unless it has tests.
> I think definitely there should be a ticket opened to ensure that
> tests are written to test the logic staff_member_required and
> request.get_full_path(). At the moment I'm not sure where they would
> fit.
The hard part is writing the tests, not deciding where to put them.
Worst case, the core developer that commits your patch will put the
tests into a different location.
However, that said, the decision on where to put a test goes something
like this:
* Is it a specific test of a contrib app? If yes, put it in the tests
module for that app
* Could the test serve as a form of documentation for a feature? If
yes, put it in modeltests.
* Otherwise, put it in regressiontests.
Beyond that, try to add the tests to an existing test module within
modeltests/regressiontests; however, if you can't find anything
appropriate, feel free to suggest a new test module.
Yours,
Russ Magee %-)
Rozza
I write the tests and attach to the ticket asap.
Ross
On Jul 18, 2:01 pm, "Russell Keith-Magee" <freakboy3...@gmail.com>
wrote:
Rozza
Shows the reason for the triage process!
All the best
Ross
Julien Phalip
Rozza
#5801 focuses on all admin logins (the original patch just looked at
the decorator though).
The new patch for #5801 focuses both on the decorated custom admin
views and normal admin views. It also normalises the behaviour
between the decorator logic and the sites.py login method as they
behaved differently.
Ross
On Jul 18, 11:07 pm, Julien Phalip <jpha...@gmail.com> wrote:
> Errr, isn't that a duplicate of #5775?http://code.djangoproject.com/ticket/5775
Rozza
It may be worth noting, the current @staff_member_required contains a
minor security breach - if you enter a valid email address then it
will output the users username even if the password is incorrect.
The attached patch behaves like the main admin login and ensures that
the email and password are valid.
Ross