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

Commencements made #1990

Merged
merged 5 commits into from
Feb 21, 2024
Merged

Commencements made #1990

merged 5 commits into from
Feb 21, 2024

Conversation

actlikewill
Copy link
Contributor

@actlikewill actlikewill commented Feb 19, 2024

@actlikewill actlikewill marked this pull request as ready for review February 19, 2024 15:41
@goose-life
Copy link
Contributor

very nice!
yes I totally agree — if there's already a main commencement on the work being commenced, we shouldn't (be able to) set either of ours. this goes for non-multiple commencements too.
if a commenced work also has already been marked as 'all provisions commenced' (which would be incorrect but possible), that could also create issues.
I'll review the code now and see if I can suggest something.

Comment on lines +589 to +591
commenced_counts = Commencement.objects.filter(commencing_work=work).values("commenced_work").annotate(num_commencements=Count("id")).order_by()
multiple_commencements = commenced_counts.filter(num_commencements__gt=1).values_list("commenced_work", flat=True)
single_commencements = commenced_counts.filter(num_commencements=1).values_list("commenced_work", flat=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

so here we should also be taking into account any other existing commencements on all the commenced works, as you rightly pointed out in your video.

  • if any of those other commencements is already main, don't set any of ours to main. if none are, we can still use the earliest (or only) one that we're adding
  • if any have all_provisions=True, we should set that to False and save that commencement, and make ours False as well

you can probably treat all 'multiple' and 'single' commencements the same, as long as

  • the single and multiple lists are based on all commencements on the commenced work (Commencement.filter(commenced_work=commenced_work))
  • the multiples logic is tweaked to check for existing main and all_provisions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goose-life
Copy link
Contributor

I think some of your logic around 'if commencement made / else commenced by' can probably come out, as we likely won't be reusing too much of the commencements for the other side of the relationship — but it doesn't hurt to leave it in for now and see, maybe we do end up reusing some of it!

@actlikewill
Copy link
Contributor Author

actlikewill commented Feb 21, 2024

I think some of your logic around 'if commencement made / else commenced by' can probably come out, as we likely won't be reusing too much of the commencements for the other side of the relationship — but it doesn't hurt to leave it in for now and see, maybe we do end up reusing some of it!

@goose-life I've removed references to the commenced_by in the code. I don't think we will use it if we do commenced_by separately.

@actlikewill actlikewill merged commit ed817f0 into master Feb 21, 2024
6 checks passed
@actlikewill actlikewill deleted the commencements branch February 21, 2024 15:03
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.

2 participants