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

creates impacts app and some models #1531

Merged
merged 4 commits into from
May 28, 2024
Merged

creates impacts app and some models #1531

merged 4 commits into from
May 28, 2024

Conversation

george-silva
Copy link
Contributor

No description provided.

SEQUENCE = "SEQUENCE", "Sequence"


class TreatmentPrescriptionAction(models.TextChoices):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

List fetched from David Schmidt

# I still can't decide if it's best for us to clone the stand geometry
# or to have a direct reference to it. I think cloning makes sense because
# it actually frees us from a lot of FK handling
stand = models.ForeignKey(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lastminutediorama @pflopez this is where I have most of my questions...

I don´t think it is really useful/necessary to have a hard foreign key to the stand. The reason is that this creates friction with the stands (let's delete all stands and recreate them, if we ever need).

If we copy the stand geometry, we don't need the FK. We might have to copy some other property like stand size.

):
created_by = models.ForeignKey(
User,
related_name="tx_plans",
Copy link
Contributor Author

@george-silva george-silva May 24, 2024

Choose a reason for hiding this comment

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

tx is a common abbreviation for treatment. if you don't like it, we can expand into the full name.

Copy link
Contributor

@pflopez pflopez left a comment

Choose a reason for hiding this comment

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

LGTM! chatted about a missing name on treatment plan, but otherwise looks great!
Regarding cloning vs FK - I leave this to you and @lastminutediorama ... my gut tells me FK are better but I don't know all the work involved into maintaining that properly. We discussed some use cases which we can handle with both solutions, so from my end, do whatever its more ergonomic for future development 🎉

Copy link
Contributor

@lastminutediorama lastminutediorama left a comment

Choose a reason for hiding this comment

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

Lgtm. I'm also not sure about how we should handle the stand FK, so I was planning to revisit the PRD but haven't had a chance since Friday.
But I also this is the sort of thing we can change if we decide to and it shouldn't block this PR.

@george-silva
Copy link
Contributor Author

Ok, perfect.

Let's merge and we see if need the FK. I don't think we will ever need it.

@george-silva george-silva merged commit 3161179 into main May 28, 2024
6 checks passed
@george-silva george-silva deleted the plan-1351 branch May 28, 2024 14:53
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.

3 participants