Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

PR eligibility should be sorted by maturity not submission date #700

Open
peternewman opened this issue Oct 19, 2020 · 5 comments
Open

PR eligibility should be sorted by maturity not submission date #700

peternewman opened this issue Oct 19, 2020 · 5 comments
Labels
bug-report Unverified user submitted bug that needs verification

Comments

@peternewman
Copy link

peternewman commented Oct 19, 2020

Describe the bug
PR eligibility should be sorted by maturity not submission date. Due to retrospective tagging in non-topic repos, submission is not always equal to maturity.

To Reproduce
Steps to reproduce the behavior:

Repo A - Tagged with hacktoberfest topic
Repo B - Untagged

  1. Submit PR i to repo A which gets merged
  2. Submit PR ii to repo B which gets merged
  3. PR ii gets tagged as hacktoberfest-accepted
  4. Submit PR iii to repo B which gets merged
  5. Submit PR iv to repo A which gets merged
  6. Submit PR v to repo A which gets merged
  7. Time passes
  8. PR iii gets tagged as hacktoberfest-accepted
  9. Time passes
  10. PRs i, ii, iv mature

Expected behavior
Currently PRs listed as the four eligible ones are i, ii, iii and iv; however PR v should be listed before iii as it will mature first as it was either merge in an hacktoberfest topic repo, or tagged hacktoberfest-accepted first.

Should show as complete when the first four PRs have matured. Should show total matured count as progress rather than total by submit date.

Additional context
I attempted a fix in #689, but it seems my Ruby-foo is pretty rubbish!

I'll find out in a few hours if this matters in terms of actual eligibility or just the maturity countdown display.

@peternewman peternewman added the bug-report Unverified user submitted bug that needs verification label Oct 19, 2020
@MattIPv4
Copy link
Member

MattIPv4 commented Oct 19, 2020

PRs in the profile are shown in chronological order before a user wins, not the order in which they will mature.

Once a user wins, the four eligible PRs that allowed them to win are frozen and displayed at the top of the profile, with all other PRs being displayed below, in chronological order.

There is a very slight edge-case where more than four PRs could become eligible before the app transitions the user to winning, in which case it will freeze the first four eligible PRs by submission date -- are you suggesting we should ensure that we freeze the first four that matured? This seems like an incredibly small edge case?

@peternewman
Copy link
Author

PRs in the profile are shown in chronological order before a user wins, not the order in which they will mature.

Agreed, I'm suggesting as a user I'm interested in the order they will mature in, rather than when I submitted them.

Once a user wins, the four eligible PRs that allowed them to win are frozen and displayed at the top of the profile, with all other PRs being displayed below, in chronological order.

Currently aside from your reassurance here, it's unclear if I need to wait two hours (until I've had four matured PRs in total) or five days when the four PRs listed as "Your Contributions" will have all matured.

There is a very slight edge-case where more than four PRs could become eligible before the app transitions the user to winning, in which case it will freeze the first four eligible PRs by submission date -- are you suggesting we should ensure that we freeze the first four that matured? This seems like an incredibly small edge case?

No, I'm just suggesting it should always show them in the order they'll mature (well at least before freezing, I'm unlikely to care after that).

From what I understand this edge case wouldn't have existed as much before the opt-in rule change as PRs could only mature in merge order although I guess an unmerged tagged one could still be non-chronological if they are sorted by creation rather than merge date.

@MattIPv4
Copy link
Member

Ah, I misunderstood then, you want all PRs on the profile to be sorted by when they will mature?

Where would you expect to see ineligible PRs -- at the top or bottom?

@peternewman
Copy link
Author

Ah, I misunderstood then, you want all PRs on the profile to be sorted by when they will mature?

Yeah, well my main thing is I think the four picked out as "Your contributions" should be the four that will make you win first. Given the way the current code works, sorting them all this way would make sense.

Where would you expect to see ineligible PRs -- at the top or bottom?

I guess the bottom, given they'll effectively never mature. Although it looks like the code currently filters out the four winning ones from the list of the others, so logically the list of non-winning/extra ones could be sorted by submission date as it is currently.

i.e. I think if my PR actually did what was intended, the page would show as I'd expect:
https://github.com/digitalocean/hacktoberfest/pull/689/files

Or in terms of my initial example, show i, ii, iv, v as "Your contributions" list iii and any other untagged/non-topic or future PRs at the bottom.

@MattIPv4
Copy link
Member

The code you've edited in your PR there is what's used for freezing the four winning PRs, nothing else.

I think there might also be some confusion here around "Your contributions" -- all PRs listed, above and below the "You're almost there!" message.

Currently, the logic for the PRs above "You're almost there!" is PRs in chronological order, until it reaches four waiting/eligible PRs: https://github.com/digitalocean/hacktoberfest/blob/e503c0dbf06e7900b9e5463425e5587e24138927/app/presenters/profile_page_presenter.rb#L50-L55

This means that, currently, if you had some ineligible PRs mixed in with your first four waiting PRs, they would all display above the "You're almost there!".

It is one continuous timeline, the message simply shows at the point where you have four eligible/waiting PRs.


Sorting the PRs by their maturity date instead of creation, with ineligible PRs being separated, would ensure that there are only ever four PRs shown above "You're almost there!", with them being the first four that will become eligible.

I don't think this is a change that will be made this year, as it will likely be confusing for the existing users (this is the first request for such a change), and in theory would be a change to the winning logic, which we don't want to adjust whilst the competition is running.

We leave this issue open though as a reminder of this suggestion for next year :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug-report Unverified user submitted bug that needs verification
Projects
None yet
Development

No branches or pull requests

2 participants