Skip to content
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.

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

Updated models.py #316

Open
wants to merge 6 commits into
base: create-market-from-sublet
Choose a base branch
from

Conversation

minghansun1
Copy link

No description provided.

Copy link
Contributor

@vcai122 vcai122 left a comment

Choose a reason for hiding this comment

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

Great start. Just be careful to make sure you aren't missing the small things from before when you rewrite the code! Looking forward to the rest of the changes!

backend/market/models.py Outdated Show resolved Hide resolved
User, through=Offer, related_name="sublets_offered", blank=True
class Item(models.Model):
seller = models.ForeignKey(User, on_delete=models.CASCADE)
tags = models.ManyToManyField(Tag, related_name="items", blank=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to specify related_name for most things unless there are multiple relationships

Copy link
Author

Choose a reason for hiding this comment

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

If we don't specify related_name, django automatically generates a related_name right?
Also, do you mean we don't need to specify related_name for most things unless there are multiple relationships with the same model in a model?

Copy link
Contributor

@vcai122 vcai122 Oct 23, 2024

Choose a reason for hiding this comment

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

Yes, django automatically generates it. For the most part we only specify it if there is a strong reason to, one example is

if model A has

  • goodBs = many to many to B
  • badBs = many to many to B

Then we have to name the related_name specifically, so if we are on B looking at A we know which one we are talking about


class Sublet(models.Model):
item = models.OneToOneField(Item, on_delete=models.CASCADE, related_name="sublet")
address = models.CharField(max_length=255)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the same requirements for the model and same model fields.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by this? Are you asking me to remove related_name again?

image = models.ImageField(upload_to="sublet/images")
class ItemImage(models.Model):
item = models.ForeignKey(Item, on_delete=models.CASCADE, related_name="images")
image = models.ImageField(upload_to="marketplace/images")
Copy link
Contributor

Choose a reason for hiding this comment

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

add a TODO here so we are reminded to verify this in production

Copy link
Author

Choose a reason for hiding this comment

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

What exactly do you want us to verify? Is it the image routes?
So i can leave a more specific TODO

Copy link
Contributor

Choose a reason for hiding this comment

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

make sure that this s3 bucket exists/if we need to make it manually or django will make it for us



class Amenity(models.Model):
class Category(models.TextChoices):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this an actual model. That way I can add more categories on admin site.

@minghansun1
Copy link
Author

Thanks for the feedback!

Copy link
Contributor

@vcai122 vcai122 left a comment

Choose a reason for hiding this comment

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

I looked at the models again but just skimmed the serializers.
I noticed some comments were unaddressed, I think a good system would be if you thumbs up the ones that are addressed (and I can verify and close the conversation). That way we know whats left to address.


user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="offers_made")
sublet = models.ForeignKey("Sublet", on_delete=models.CASCADE, related_name="offers")
user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="offers_made_market")
Copy link
Contributor

Choose a reason for hiding this comment

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

are these _market because sublet exists? If so let's make a note to fix this after we delete sublet stuff

Copy link
Author

Choose a reason for hiding this comment

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

I'll just remove related_name since there's only one ForeignKey to User in Offer.



class Amenity(models.Model):
class Category(models.Model):
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove this comment. feel free to post it in our slack chat or smth if u want to keep it around somewhere but lets not have dead code

Copy link
Author

Choose a reason for hiding this comment

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

I kept it in so we remember which Categories to create when deploying the app. But yeah keeping it in Slack will be cleaner.

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

Successfully merging this pull request may close these issues.

2 participants