Skip to content

Adding delete to ProfileAvatar #72

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.

Sign up for GitHub

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

Jump to bottom
Merged
LilyFirefly merged 12 commits into from
Oct 28, 2014
Merged

Adding delete to ProfileAvatar #72

LilyFirefly merged 12 commits into from
Oct 28, 2014

Conversation

@SemVerTsar
Copy link
Contributor

@SemVerTsar SemVerTsar commented Oct 24, 2014

No description provided.

@SemVerTsar
Added delete view
818d0f7
@SemVerTsar
Added serializer to view
5d7d144
@SemVerTsar
Added tests for delete
075830a 
added get user to view
@SemVerTsar
Pep8 fixes
98c81cc 
&
make tests consistant
@SemVerTsar
Profile avatar url refactored
26b974d 
Avatar Delete refactor

Tests changed
@SemVerTsar
Copy link
Contributor Author

SemVerTsar commented Oct 24, 2014

@meshy Please review

@meshy
Copy link
Contributor

meshy commented Oct 24, 2014

Tests fail

@meshy
Copy link
Contributor

meshy commented Oct 24, 2014

I've restarted the build

@coveralls
Copy link

coveralls commented Oct 24, 2014

Coverage Status

Coverage remained the same when pulling 26b974d on avatar-delete into 27c528a on master.

meshy
meshy reviewed Oct 24, 2014
View reviewed changes
user_management/api/avatar/tests/test_views.py Outdated
Copy link
Contributor

@meshy meshy Oct 24, 2014

Choose a reason for hiding this comment

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

Is this step required?
user = User.objects.get(pk=user.pk)

Copy link
Contributor Author

@SemVerTsar SemVerTsar Oct 24, 2014

Choose a reason for hiding this comment

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

No, I missed it

@LilyFirefly
Copy link

LilyFirefly commented Oct 24, 2014

The ProfileAvatar view's docstring should mention the delete functionality.

@SemVerTsar
Removed pointless user get
a0910ae 
Added assert for avatar = None
@coveralls
Copy link

coveralls commented Oct 24, 2014

Coverage Status

Coverage remained the same when pulling a0910ae on avatar-delete into 27c528a on master.

@SemVerTsar
Copy link
Contributor Author

SemVerTsar commented Oct 24, 2014

Bump

LilyFirefly
LilyFirefly reviewed Oct 24, 2014
View reviewed changes
user_management/api/avatar/tests/test_views.py Outdated
Copy link

@LilyFirefly LilyFirefly Oct 24, 2014

Choose a reason for hiding this comment

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

Stray print here.

Copy link
Contributor Author

@SemVerTsar SemVerTsar Oct 24, 2014

Choose a reason for hiding this comment

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

dang, sorry it's friday!

@coveralls
Copy link

coveralls commented Oct 24, 2014

Coverage Status

Coverage remained the same when pulling 8214bee on avatar-delete into 27c528a on master.

@coveralls
Copy link

coveralls commented Oct 24, 2014

Coverage Status

Coverage remained the same when pulling 1a96872 on avatar-delete into 27c528a on master.

@kevinetienne
Copy link
Contributor

kevinetienne commented Oct 24, 2014

Sorry still looks like you need the _commited attribute

@coveralls
Copy link

coveralls commented Oct 24, 2014

Coverage Status

Coverage remained the same when pulling f594f7b on avatar-delete into 27c528a on master.

@SemVerTsar
Copy link
Contributor Author

SemVerTsar commented Oct 24, 2014

Merge?

@LilyFirefly
Copy link

LilyFirefly commented Oct 24, 2014

You haven't responded to this comment.

SemVerTsar
SemVerTsar reviewed Oct 24, 2014
View reviewed changes
user_management/api/avatar/views.py
Copy link
Contributor Author

@SemVerTsar SemVerTsar Oct 24, 2014

Choose a reason for hiding this comment

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

@ian-foote I updated the doc string here. I read that comment but there was nothing for me to add.

@SemVerTsar
Copy link
Contributor Author

SemVerTsar commented Oct 28, 2014

@ian-foote Please take another look

@coveralls
Copy link

coveralls commented Oct 28, 2014

Coverage Status

Coverage remained the same when pulling f8bbb12 on avatar-delete into 27c528a on master.

@SemVerTsar
Copy link
Contributor Author

SemVerTsar commented Oct 28, 2014

@ian-foote ?

@coveralls
Copy link

coveralls commented Oct 28, 2014

Coverage Status

Coverage remained the same when pulling 53fc2bd on avatar-delete into 27c528a on master.

LilyFirefly pushed a commit that referenced this pull request Oct 28, 2014
Merge pull request #72 from incuna/avatar-delete
b94a288 
Adding delete to ProfileAvatar
@LilyFirefly LilyFirefly merged commit b94a288 into master Oct 28, 2014
@LilyFirefly LilyFirefly deleted the avatar-delete branch October 28, 2014 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@SemVerTsar @meshy @coveralls @LilyFirefly @kevinetienne