Please Apply #5801 - GET parameters are ignored in redirect for Admin

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Please Apply #5801 - GET parameters are ignored in redirect for Admin

Ross Lawley-2

Hi all,

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
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Russell Keith-Magee-2

On Fri, Jul 18, 2008 at 3:57 PM, Rozza <[hidden email]> wrote:
>
> Hi all,
>
> 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!

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 %-)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Ross Lawley-2

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?

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" <[hidden email]>
wrote:

> On Fri, Jul 18, 2008 at 3:57 PM, Rozza <[hidden email]> wrote:
>
> > Hi all,
>
> > 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!
>
> 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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Russell Keith-Magee-2

On Fri, Jul 18, 2008 at 8:47 PM, Rozza <[hidden email]> wrote:
>
> 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 %-)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Ross Lawley-2

Thanks Russell,

I write the tests and attach to the ticket asap.

Ross

On Jul 18, 2:01 pm, "Russell Keith-Magee" <[hidden email]>
wrote:

> On Fri, Jul 18, 2008 at 8:47 PM, Rozza <[hidden email]> wrote:
>
> > 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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Ross Lawley-2

New patch with tests added to the ticket.

Shows the reason for the triage process!

All the best

Ross

On Jul 18, 2:54 pm, Rozza <[hidden email]> wrote:

> Thanks Russell,
>
> I write the tests and attach to the ticket asap.
>
> Ross
>
> On Jul 18, 2:01 pm, "Russell Keith-Magee" <[hidden email]>
> wrote:
>
> > On Fri, Jul 18, 2008 at 8:47 PM, Rozza <[hidden email]> wrote:
>
> > > 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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Julien Phalip-2

Errr, isn't that a duplicate of #5775?
http://code.djangoproject.com/ticket/5775

On Jul 19, 6:19 am, Rozza <[hidden email]> wrote:

> New patch with tests added to the ticket.
>
> Shows the reason for the triage process!
>
> All the best
>
> Ross
>
> On Jul 18, 2:54 pm, Rozza <[hidden email]> wrote:
>
> > Thanks Russell,
>
> > I write the tests and attach to the ticket asap.
>
> > Ross
>
> > On Jul 18, 2:01 pm, "Russell Keith-Magee" <[hidden email]>
> > wrote:
>
> > > On Fri, Jul 18, 2008 at 8:47 PM, Rozza <[hidden email]> wrote:
>
> > > > 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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Ross Lawley-2

Yeah looks like it except #5775 just focuses on the decorator and
#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 <[hidden email]> wrote:

> Errr, isn't that a duplicate of #5775?http://code.djangoproject.com/ticket/5775
>
> On Jul 19, 6:19 am, Rozza <[hidden email]> wrote:
>
> > New patch with tests added to the ticket.
>
> > Shows the reason for the triage process!
>
> > All the best
>
> > Ross
>
> > On Jul 18, 2:54 pm, Rozza <[hidden email]> wrote:
>
> > > Thanks Russell,
>
> > > I write the tests and attach to the ticket asap.
>
> > > Ross
>
> > > On Jul 18, 2:01 pm, "Russell Keith-Magee" <[hidden email]>
> > > wrote:
>
> > > > On Fri, Jul 18, 2008 at 8:47 PM, Rozza <[hidden email]> wrote:
>
> > > > > 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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply | Threaded
Open this post in threaded view
|

Re: Please Apply #5801 - GET parameters are ignored in redirect for Admin

Ross Lawley-2


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

On Jul 19, 8:43 am, Rozza <[hidden email]> wrote:

> Yeah looks like it except #5775 just focuses on the decorator and
> #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 <[hidden email]> wrote:
>
> > Errr, isn't that a duplicate of #5775?http://code.djangoproject.com/ticket/5775
>
> > On Jul 19, 6:19 am, Rozza <[hidden email]> wrote:
>
> > > New patch with tests added to the ticket.
>
> > > Shows the reason for the triage process!
>
> > > All the best
>
> > > Ross
>
> > > On Jul 18, 2:54 pm, Rozza <[hidden email]> wrote:
>
> > > > Thanks Russell,
>
> > > > I write the tests and attach to the ticket asap.
>
> > > > Ross
>
> > > > On Jul 18, 2:01 pm, "Russell Keith-Magee" <[hidden email]>
> > > > wrote:
>
> > > > > On Fri, Jul 18, 2008 at 8:47 PM, Rozza <[hidden email]> wrote:
>
> > > > > > 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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to [hidden email]
To unsubscribe from this group, send email to [hidden email]
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---