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

Add contribution timeline #2617

Merged
merged 17 commits into from
Sep 8, 2022
Merged

Conversation

mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Sep 6, 2022

Fixes #2487 definitively.

The patch implements contribution timeline, which can be filtered by contribution type and day.

It's deployed to stage (which uses ~2 weeks old prod data):
https://mozilla-pontoon-staging.herokuapp.com/contributors/93Rx_ofmgBDhnz4QZD1aHlOg0uY/

TODO:

@mathjazz
Copy link
Collaborator Author

mathjazz commented Sep 6, 2022

@flodolo Could you take this for a spin on stage?

@flodolo
Copy link
Collaborator

flodolo commented Sep 7, 2022

There are a couple of differences compared to specs:

  • There is no selector for the year
  • You can select a day, but not a week

I feel like we might have talked about the years stuff, but I'm not completely sure at this point.

Are these too complex to implement?

The rest looks great to me.

@mathjazz
Copy link
Collaborator Author

mathjazz commented Sep 7, 2022

There are a couple of differences compared to specs:

* There is no selector for the year
* You can select a day, but not a week

I feel like we might have talked about the years stuff, but I'm not completely sure at this point.

Yes, we talked about postponing the year selector: I've added a TODO item to file an issue for that. Using the day selector instead of a week selector was a conscious decision that I forgot to explain, so let me do that now.

During the spec phase we considered using a week as the smallest unit rendered on the contribution graph instead of a day, because we didn't know how the daily graphs will turn out with the actual data. In that scenario, having a weekly selector would make sense.

We went ahead with a day as the smallest graph unit, so being able to filter contributions by a day is the most intuitive option. Otherwise rendering each day on the graph doesn't make that much sense. I would even argue that monthly selector makes more sense in the current graph, if we make month labels clickable.

@mathjazz mathjazz requested a review from eemeli September 7, 2022 12:12
Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Looks good; a couple of minor things inline to resolve.

Comment on lines +373 to +380
contributions_qs.annotate(
project_name=F("translation__entity__resource__project__name"),
project_slug=F("translation__entity__resource__project__slug"),
locale_name=F("translation__locale__name"),
locale_code=F("translation__locale__code"),
)
.values("project_name", "project_slug", "locale_name", "locale_code")
.annotate(count=Count("id"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
contributions_qs.annotate(
project_name=F("translation__entity__resource__project__name"),
project_slug=F("translation__entity__resource__project__slug"),
locale_name=F("translation__locale__name"),
locale_code=F("translation__locale__code"),
)
.values("project_name", "project_slug", "locale_name", "locale_code")
.annotate(count=Count("id"))
contributions_qs.annotate(
project_name=F("translation__entity__resource__project__name"),
project_slug=F("translation__entity__resource__project__slug"),
locale_name=F("translation__locale__name"),
locale_code=F("translation__locale__code"),
count=Count("id"),
)

Would this also work? Not sure tbh, but as is the code looks awfully repetitive.

Copy link
Collaborator Author

@mathjazz mathjazz Sep 8, 2022

Choose a reason for hiding this comment

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

Sadly no. This is a very implicit feature of the Django ORM to make annotations over unique groups.

To quote the docs:

Ordinarily, annotations are generated on a per-object basis - an annotated QuerySet will return one result for each object in the original QuerySet. However, when a values() clause is used to constrain the columns that are returned in the result set, the method for evaluating annotations is slightly different. Instead of returning an annotated result for each result in the original QuerySet, the original results are grouped according to the unique combinations of the fields specified in the values() clause. An annotation is then provided for each unique group; the annotation is computed over all members of the group.

Comment on lines 452 to 454
"user_translations": f"author={ email }&time={ start_ }-{ end_ }",
"user_reviews": f"reviewer={ email }&review_time={ start_ }-{ end_ }",
"peer_reviews": f"author={ email }&review_time={ start_ }-{ end_ }&exclude_self_reviewed",
Copy link
Member

Choose a reason for hiding this comment

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

These should use urllib.urlencode.

@mathjazz mathjazz merged commit f1a69e8 into mozilla:master Sep 8, 2022
@mathjazz mathjazz deleted the 2487-contributor-timeline branch September 8, 2022 19:12
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.

Implement contribution graph
3 participants