Skip to content

Add user association to a rover#74

Merged
hbradio merged 1 commit into
rovercode:developmentfrom
cabarnes:rover-user-association
May 30, 2017
Merged

Add user association to a rover#74
hbradio merged 1 commit into
rovercode:developmentfrom
cabarnes:rover-user-association

Conversation

@cabarnes

@cabarnes cabarnes commented May 30, 2017

Copy link
Copy Markdown
Member

Related to #19 in order to modify the user's rovers. Changes owner to be an actual user. There's no easy migration from a string to a foreign key, so the Rover objects will all need to be removed when the migrations are applied.

@cabarnes cabarnes added this to the Release 0.5.0 milestone May 30, 2017
@cabarnes cabarnes requested a review from hbradio May 30, 2017 02:57
@coveralls

coveralls commented May 30, 2017

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling f5b91a5 on cabarnes:rover-user-association into d80efc0 on aninternetof:development.

hbradio
hbradio approved these changes May 30, 2017
View reviewed changes

@hbradio hbradio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

Comment thread mission_control/serializers.py


class RoverSerializer(serializers.HyperlinkedModelSerializer):
class RoverSerializer(serializers.ModelSerializer):

@hbradio hbradio May 30, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm reading about the differences between these two. The HyperlinkedModelSerializer does not lend itself to use with a foreign key?

@cabarnes cabarnes May 30, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The issue I was running into was that the User model didn't have a serializer. If we eventually want to implement that we can put this back. It just didn't seem all that important.

@hbradio hbradio May 30, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah! That makes sense.

@hbradio

hbradio commented May 30, 2017

Copy link
Copy Markdown
Collaborator

I went to https://beta.rovercode.com/mission-control/rovers/ to delete the rovers before we merge this PR. But, I'm getting a 500 error. Was this expected from previous changes? (I didn't think anything about the model changed in the previous PR).
I can always delete them within Django, but I'm surprised we didn't have a test for this DRF view.
If I get a minute over lunch, I'll SSH into beta.rovercode.com and try to see what's up.

@cabarnes

cabarnes commented May 30, 2017

Copy link
Copy Markdown
Member Author

I wouldn't have expected an issue. Everything is still running locally for me. The only change to the model recently was adding the pin assignment fields which was a migration.

@hbradio

hbradio commented May 30, 2017

Copy link
Copy Markdown
Collaborator

Huh, interesting.

And actually, there shouldn't be any rover records, because on every view, they get deleted if they haven't check in recently.

@hbradio

hbradio commented May 30, 2017

Copy link
Copy Markdown
Collaborator

I'll go ahead an merge this PR and see where we end up.

@hbradio hbradio merged commit cca0bc6 into rovercode:development May 30, 2017
@cabarnes cabarnes deleted the rover-user-association branch June 4, 2017 02:35
@hbradio hbradio mentioned this pull request Jun 27, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hbradio hbradio hbradio approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Release 0.5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@cabarnes @coveralls @hbradio