Skip to content

Conversation

@yakky
Copy link
Member

@yakky yakky commented Jan 12, 2015

Fix #3672
Fix #3659

@yakky yakky added this to the 3.0.10 milestone Jan 12, 2015
@yakky
Copy link
Member Author

yakky commented Jan 15, 2015

This will require a different implementation for 3.1

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling f583f7b on yakky:feature/fix_3672 into b24b32f on divio:support/3.0.x.

@coveralls
Copy link

coveralls commented Jan 15, 2015

Coverage Status

Coverage decreased (-0.1%) to 87.489% when pulling 500607e on yakky:feature/fix_3672 into b24b32f on divio:support/3.0.x.

@yakky
Copy link
Member Author

yakky commented Jan 16, 2015

@mkoistinen would you check this? I'd like to avoid another regression in the toolbar for the same bug :D

@jsma
Copy link
Contributor

jsma commented Jan 24, 2015

I just tried to reproduce #3659 on the latest in the develop branch (haven't tried this PR) but I can't reproduce the bug anymore, logging out works. I haven't followed recent developments in develop the past few weeks but perhaps something there fixed this issue already? Or were your tests here failing without your patch to toolbar.py? Either way, thanks!

I started working on a test case for this but turned into an exercise in yak shaving (mostly due to my unfamiliarity with how the django-cms test suite is organized) and wasn't able to complete. Looks like I can delete that git stash now and I can learn from your example here instead for next time, thanks again!

@yakky
Copy link
Member Author

yakky commented Jan 24, 2015

Actually no changes directly related to this in develop.
I'll rebase the test to check.
The toolbar framework will go through a bit of refactoring in develop, so thi will be fixed in quite a different way

Copy link
Contributor

Choose a reason for hiding this comment

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

Some may see this as "passive aggressive". "Web site" should probably just be "website".

Perhaps we should just say something to confirm the action: "You have successfully logged-out." perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the standard django logout message
This template it's just for testing purposes, it's never shown to the user

@jsma
Copy link
Contributor

jsma commented Jan 30, 2015

Haven't investigated further yet but did notice that when I logout, the toolbar appears on the logged out template with the login form displayed in the toolbar. Navigate to another page, the toolbar is cleared. So maybe something changed the logic so it smartly shows the login form instead of assuming a logged in user which resulted in the exception I experienced previously. Again, this is only on the develop branch.

@jsma
Copy link
Contributor

jsma commented Feb 11, 2015

As mentioned above the bug was apparently fixed through an unrelated change. It's been too long since I've looked at this code so have no comment on the small change to the "is_staff" check but the tests will be good to have to prevent regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants