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

PNI: Detail page WIP #1846

Merged
merged 24 commits into from
Sep 28, 2018
Merged

PNI: Detail page WIP #1846

merged 24 commits into from
Sep 28, 2018

Conversation

gvn
Copy link
Contributor

@gvn gvn commented Sep 21, 2018

No description provided.

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-1846 September 21, 2018 21:39 Inactive
@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 21, 2018 21:53 Inactive
@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 21, 2018 22:53 Inactive
@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 21, 2018 22:54 Inactive
@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 21, 2018 23:04 Inactive
@gvn
Copy link
Contributor Author

gvn commented Sep 24, 2018

@kristinashu
Copy link

kristinashu commented Sep 25, 2018

To move this ticket forward, I'm approving the design.

We have design QA notes are in this doc which I will put into a separate follow up ticket.

@gvn gvn requested a review from alanmoo September 25, 2018 18:06
@gvn
Copy link
Contributor Author

gvn commented Sep 25, 2018

@alanmoo in the interest of keeping code reviews on the smaller side I think we should land this chunk. Have a look see and let me know if there are any glaring issues bearing in mind that it's very WIP.

@kristinashu
Copy link

I've put QA list in a ticket #1860

@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 25, 2018 21:58 Inactive
@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 25, 2018 21:58 Inactive
@alanmoo alanmoo temporarily deployed to foundation-mofostaging-pr-1846 September 26, 2018 16:12 Inactive
Copy link
Contributor

@alanmoo alanmoo left a comment

Choose a reason for hiding this comment

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

Some comments, please respond if you want separate tickets for them.

2 blockers to land this:

  1. Fix Travis. I think it's just syntax.
  2. Squash migrations

menu_label = 'Updates'
menu_icon = 'pick' # change as required
menu_order = 600 # will put in 3rd place (000 being 1st, 100 2nd)
add_to_settings_menu = False # or True to add your model to the Settings sub-menu
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but let's investigate putting the buyer's guide models into a folder in the sidebar so they're nicely contained when visible.

@@ -144,6 +233,42 @@ class Product(models.Model):
blank="True",
)

update1 = models.ForeignKey(
Copy link
Contributor

Choose a reason for hiding this comment

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

When picking an update, the select element simply shows "Update object". Let's get the title of the update to show up instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you happen to know how to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NM got it

@@ -144,6 +233,42 @@ class Product(models.Model):
blank="True",
)

update1 = models.ForeignKey(
Copy link
Contributor

Choose a reason for hiding this comment

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

This also feels like a weird way to implement this. @gideonthomas do you happen to know of a DRYer, simple way to do this?

Copy link
Contributor

@gideonthomas gideonthomas Sep 26, 2018

Choose a reason for hiding this comment

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

Yeah don't create multiple FK properties on models. How you implement this depends on what the scenario here is:

A product can contain multiple updates, and multiple products can contain the same update:

class Product(models.Model):
    ...
    updates = models.ManyToManyField(Update, related_name='products', null=True)

A product can contain multiple updates, and an update is unique to (and belongs to) a single product (multiple products cannot have the same update):

class Update(models.Model):
    ...
    product = models.ForeignKey(Product, related_name='updates')

A product can contain multiple updates IN ORDER, and multiple products can contain the same update:
This is a little more work since you'll have to add logic to set the order everytime you add a new update to a product (using set_update_order() - docs)

class ProductUpdate(models.Model):
    product = models.ForeignKey(Product, on_delete=models.CASCADE)
    update = models.ForeignKey(Update, on_delete=models.CASCADE)

    class Meta:
        order_with_respect_to = 'product'
        unique_together = ('product', 'update',)
        # You will need to add this next property in a new migration after you migrate the rest of this model
        indexes = [
            models.Index(fields=['product', '_order'], name='uk_productupdate_productid_order'),
            models.Index(files=['product', 'update'], name='uk_productupdate_product_update')
        ]

A product can contain multiple updates IN ORDER, and an update is unique to (and belongs to) a single product (multiple products cannot have the same update):

class Update(models.Model):
    ...
    product = models.ForeignKey(Product, related_name='updates')
    
    class Meta:
        order_with_respect_to = 'product'
        # You will need to add this next property in a new migration after you migrate the rest of this model
        indexes = [
            models.Index(fields=['product', '_order'], name='uk_update_productid_order')
        ]

@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 27, 2018 20:44 Inactive
@gvn gvn temporarily deployed to foundation-mofostaging-pr-1846 September 27, 2018 20:57 Inactive
@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-1846 September 27, 2018 21:11 Inactive
@gvn gvn requested a review from alanmoo September 27, 2018 21:20
@gvn gvn merged commit 0599f2f into master Sep 28, 2018
@Pomax Pomax deleted the pni2 branch October 30, 2018 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants