Skip to content

Introduce Drafts page and automatic CF creation #70

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.

Sign up for GitHub

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

Merged
JelteF merged 8 commits into from
Jun 15, 2025
Merged

Introduce Drafts page and automatic CF creation #70

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
90792d1
Introduce Drafts page and automatic CF creation
JelteF Jun 9, 2025
570b787
Don't automatically create commitfests in dev environments
JelteF Jun 9, 2025
975e365
Add "Returned with Feedback" and expand explanation of other patch st…
JelteF Jun 9, 2025
4fd357f
Add commet about reasoning why we update commitfests in this kinda we…
JelteF Jun 9, 2025
a29aa00
Missing word in help page
JelteF Jun 9, 2025
4c4a2c6
Automatically move patches in open CF to in progress CF if they get c…
JelteF Jun 9, 2025
f6c61e7
Fix bug setting committer
JelteF Jun 9, 2025
96b0469
Automatically move committed draft patches to the open commitfest
JelteF Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions pgcommitfest/commitfest/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,10 @@ def next_draft_cf(start_date):
draft=True,
)

@classmethod
def get_in_progress(cls):
return cls.objects.filter(status=CommitFest.STATUS_INPROGRESS).first()

def __str__(self):
return self.name

Expand Down Expand Up @@ -406,7 +410,9 @@ def update_lastmail(self):
else:
self.lastmail = max(threads, key=lambda t: t.latestmessage).latestmessage

def move(self, from_cf, to_cf):
def move(self, from_cf, to_cf, by_user, allow_move_to_in_progress=False):
"""Returns the new PatchOnCommitFest object, or raises UserInputError"""

current_poc = self.current_patch_on_commitfest()
if from_cf.id != current_poc.commitfest.id:
raise UserInputError("Patch not in source commitfest.")
Expand All @@ -423,7 +429,10 @@ def move(self, from_cf, to_cf):
f"Patch in state {current_poc.statusstring} cannot be moved."
)

if not to_cf.is_open:
if to_cf.is_in_progress:
if not allow_move_to_in_progress:
raise UserInputError("Patch can only be moved to an open commitfest")
elif not to_cf.is_open:
raise UserInputError("Patch can only be moved to an open commitfest")

old_status = current_poc.status
Expand All @@ -443,6 +452,14 @@ def move(self, from_cf, to_cf):
self.set_modified()
self.save()

PatchHistory(
patch=self,
by=by_user,
what=f"Moved from CF {from_cf} to CF {to_cf}",
).save_and_notify()

return new_poc

def __str__(self):
return self.name

Expand Down
19 changes: 12 additions & 7 deletions pgcommitfest/commitfest/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,17 @@ def close(request, patchid, status):
by=request.user,
what="Changed committer to %s" % committer,
).save_and_notify(prevcommitter=prevcommitter)

if poc.is_open:
Copy link
Copy Markdown
Contributor

@polobo polobo Jun 9, 2025

Choose a reason for hiding this comment

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

I wanted to make it a policy that committed patches never appear in Drafts - really just open and withdrawn should (though maybe rejected/RfW too - but these likely won't happen in practice anyway). I was considering it acceptable for a non-draft open CF to have commits. Thus a committed patch not within the in progress cf should be moved there if it exists. Otherwise a draft patch should be moved to the current non-draft open cf and then committed there. (Since we report end-of-CF metrics moving stuff into a closed CF should be avoided.)

Copy link
Copy Markdown
Collaborator Author

@JelteF JelteF Jun 9, 2025

Choose a reason for hiding this comment

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

That's not what you said before

We push patches written during an in progress cf all the time. Those should be able to be added to the in progress cf by a committer so their cf of record is correct. I.e., move to should show in progress conditionally.

But regarding this:

Since we report end-of-CF metrics moving stuff into a closed CF should be avoided.

At the last developer meeting many committers were even in favor of allowing people to add patches to an in-progress commitfest. I think moving committed patches there is pretty reasonable. It seems silly and confusing to say that it was committed in the later commitfest, even if it actually managed to get in during the previous one. It seems especially confusing for the final commitfest of the year, i.e. some patches would show up as being committed as part of PG19-1, but are actually part of PG18.

I'll call his out though in the announcment email on the mailinglist. If enough people think this is wrong I'll revert.

Copy link
Copy Markdown
Contributor

@polobo polobo Jun 9, 2025

Choose a reason for hiding this comment

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

"moving stuff into a closed CF" literally means the CF entry has a status of "Closed" - not "In Progress". We can move stuff into an "In Progress" status CF just fine - if one exists, which it doesn't half of the time... - in which case Draft patches being committed must first be moved to the guaranteed "Open" status CF (they likely are there already if being committed - we could instead prohibit "Commit" action while a patch is in a Draft CF.

Copy link
Copy Markdown
Contributor

@polobo polobo Jun 9, 2025

Choose a reason for hiding this comment

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

I'm OK with moving stuff into a "Closed" CF actually; figured others wouldn't be though.

Copy link
Copy Markdown
Collaborator Author

@JelteF JelteF Jun 9, 2025

Choose a reason for hiding this comment

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

Ah alright, I misunderstood/misread. Let me restate what you're saying so I'm sure I understand correctly now: The current logic is good, but it should be amended that if something is committed in a Draft it should be moved to the "In Progress" commitfest (which already happens now), and if that doesn't exist it should be moved to the regular "Open" commitfest.

Copy link
Copy Markdown
Contributor

@polobo polobo Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Put differently - should we have, right now, a "PG18-Beta" CF that is "In Progress" that all these new commits being made today get recorded within? Or do they get committed to PG18-Final (Closed) or PG19-1 (Open)?

Copy link
Copy Markdown
Contributor

@polobo polobo Jun 9, 2025

Choose a reason for hiding this comment

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

if something is committed in a Draft it should be moved to the "In Progress" commitfest (which already happens now), and if that doesn't exist it should be moved to the regular "Open" commitfest.

Yes.

Copy link
Copy Markdown
Collaborator Author

@JelteF JelteF Jun 9, 2025

Choose a reason for hiding this comment

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

Yes.

I implemented that now.

should we have, right now, a "PG18-Beta" CF that is "In Progress" that all these new commits being made today get recorded within? or PG19-1 (Open)?

I think that's an interesting question. I think immediately creating an "In Progress", "PG18-Beta" CF makes sense once the "PG18-Final" CF is closed.

Or do they get committed to PG18-Final (Closed)

I think committing into an actually closed CF seems unexpected/strange (like you also said before).

or PG19-1 (Open)?

I think it makes sense to stick with this for now though, since it's the status quo. And I wouldn't want to introduce a Beta CF now, maybe for PG19 though.

Copy link
Copy Markdown
Contributor

@polobo polobo Jun 9, 2025

Choose a reason for hiding this comment

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

Agreed with this being a year out; was using PG18 as an easy example.

in_progress_cf = CommitFest.get_in_progress()
if in_progress_cf is not None:
poc = patch.move(
poc.commitfest,
in_progress_cf,
request.user,
allow_move_to_in_progress=True,
)

poc.status = PatchOnCommitFest.STATUS_COMMITTED

status_mapping = {
Expand Down Expand Up @@ -1098,17 +1109,11 @@ def move(request, patchid):

patch = get_object_or_404(Patch, pk=patchid)
try:
patch.move(from_cf, to_cf)
patch.move(from_cf, to_cf, request.user)
except UserInputError as e:
messages.error(request, f"Failed to move patch: {e}")
return HttpResponseRedirect(f"/patch/{patchid}/")

PatchHistory(
patch=patch,
by=request.user,
what=f"Moved from CF {from_cf} to CF {to_cf}",
).save_and_notify()

return HttpResponseRedirect(f"/patch/{patchid}/")


Expand Down