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 button to show older activity in activity log #3255

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

harmitgoswami
Copy link
Collaborator

Fix #2622

Implemented a button to show more (i.e. older) activity in the user profile activity log. This feature was achieved by modifying the existing get_contribution_timeline_data function to retrieve data from the past N months, where N is a counter that the user can increase by one via clicking on the "Show more activity" button. Once the number of months shown on the activity log predates the user's account creation date, the "Show more activity" button is hidden, similar to how GitHub implements this feature.

To reproduce results: After starting the server, navigate to any user's profile (localhost:8000/contributors/[username]). Once there, click the show more activity button to display an additional month's worth of data. When choosing to view a specific contribution type, the counter resets to 0, and the "Show more activity" button is displayed again (if it was hidden before).

image
image

pontoon/contributors/static/js/profile.js Outdated Show resolved Hide resolved
pontoon/contributors/static/css/profile.css Outdated Show resolved Hide resolved
pontoon/contributors/static/css/profile.css Outdated Show resolved Hide resolved
pontoon/contributors/static/css/profile.css Outdated Show resolved Hide resolved
@mathjazz
Copy link
Collaborator

@flodolo Could you please test this feature on stage to verify it indeed fixes the bug?

Note that behavior on GitHub is different - rather than expanding the observed period by 1 month on each click, the button on GitHub appends 1-month period to the timeline.

@flodolo
Copy link
Collaborator

flodolo commented Jun 14, 2024

I think it's a bit confusing.

https://mozilla-pontoon-staging.herokuapp.com/contributors/flod/

  1. I click the button, it shows more activity (Contribution activity in the last 2 months), so that works.
  2. I click another time, nothing happens, because there is no activity between month 2 and 3 (only the title changes).
  3. I click a third time and the content changes because there is activity between month 3 and 4.

When I click the button, I would expect more activity to be loaded if available (e.g. load X more events, not expand the time filter).

It's even more confusing when I select a day with activity, e.g. in February, and then click the button.

@mathjazz
Copy link
Collaborator

When I click the button, I would expect more activity to be loaded if available (e.g. load X more events, not expand the time filter).

It's even more confusing when I select a day with activity, e.g. in February, and then click the button.

@harmitgoswami Can we change the logic to load the data from the beginning of the preceding month? So if the currently displayed data ends in June 10, we load data from May 1 to June 10 and append it. We'd also need to add month indicators to the timeline.

@harmitgoswami
Copy link
Collaborator Author

That makes sense to me. So then do we still want the button to add another month to the timeline per click, or like Flod suggested, retrieve latest X events?

@mathjazz
Copy link
Collaborator

That makes sense to me. So then do we still want the button to add another month to the timeline per click, or like Flod suggested, retrieve latest X events?

What flod suggests would be even better - so you don't click in vain.

@mathjazz
Copy link
Collaborator

Thanks for the update!

When I test the patch locally, my profile page says "Contribution activity since May 2024: ". It's not clear why since May and if that's since the beginning or since the end of May. My original suggestion was just for the data that we load additionally with the button. Sorry for not making that clearer.

Ideally, I'd refactor get_contribution_timeline_data() to work the way it works currently on page load, and to append additional data on each click on the "Show more activity" button.

@mathjazz
Copy link
Collaborator

mathjazz commented Jun 17, 2024

When I click the button, I would expect more activity to be loaded if available (e.g. load X more events, not expand the time filter).

@flodolo It turns out it'll be much easier for us to change the functionality to load events for the current month initially, and then append events for each preceding month after clicking the Show more activity button. That's also the behavior on the GH profile page. For less active users, that could mean no new events will be loaded after clicking the button, which we could address subsequently by automatically extending the period on the backend if no events are returned. Thoughts?

@flodolo
Copy link
Collaborator

flodolo commented Jun 17, 2024

By default, we're showing the graph for the last 12 months, the activity below should match that interval of time.

  • We load the activity for the last month by default.
  • Clicking Show more activity should load some data (if the previous month is empty, we should load stuff until we have something). I don't have a strong preference between "load 1 more month" vs "load X more events".
  • We stop when we reach the end of the interval (while I have the feeling now we're not stopping at all, I got to Contribution activity in the last 13 months on stage).
@mathjazz
Copy link
Collaborator

mathjazz commented Jun 17, 2024

@flodolo How about the button then just loads all the remaining events rendered on the graph, i.e. for the last 365 days?

We can also group events by month in the timeline.

@flodolo
Copy link
Collaborator

flodolo commented Jun 17, 2024

@flodolo How about the button then just loads all the remaining events rendered on the graph, i.e. for the last 365 days?

We can also group events by month in the timeline.

This works for me. And grouping would help visually.

@harmitgoswami harmitgoswami marked this pull request as draft June 20, 2024 16:46
@harmitgoswami
Copy link
Collaborator Author

Not sure what the best way is to display this information... any thoughts?

@flodolo
Copy link
Collaborator

flodolo commented Jun 22, 2024

Not sure what the best way is to display this information... any thoughts?

Can you expand on "this information"?

@harmitgoswami
Copy link
Collaborator Author

Sorry, forgot to add the images!

This order of information makes the most sense to me (Title -> Project/Locale -> Month -> Action), but I was wondering what you two thought

Initial contents:
image

After clicking "Show more activity":
image

@flodolo
Copy link
Collaborator

flodolo commented Jun 22, 2024

I think we need @mathjazz here, to avoid going too far in the wrong direction, but this looks like too much data.

My expectation is that we keep the current structure, but group it by month (i.e. take the current look of activities, repeat it for each month if there is data).

@mathjazz
Copy link
Collaborator

Agreed with Flod.

And we should follow the GitHub profile page UI - add "Month YEAR" (e.g. May 2024) to the vertical timeline.

@harmitgoswami
Copy link
Collaborator Author

In this design iteration, it's a little difficult to discern which link you're clicking on when hovering over an action. I've reserved fixing this bug for now in case this design isn't what we were looking for either.

image
@mathjazz
Copy link
Collaborator

This is the design we're aiming for:

Posnetek zaslona 2024-06-24 ob 23 59 52

@eemeli
Copy link
Member

eemeli commented Jun 27, 2024

@harmitgoswami This is currently marked as a draft PR; could you clarify what sort of review you'd like of it, and/or what's still intended to be done to it?

@harmitgoswami
Copy link
Collaborator Author

harmitgoswami commented Jun 27, 2024

@harmitgoswami This is currently marked as a draft PR; could you clarify what sort of review you'd like of it, and/or what's still intended to be done to it?

Hi Eemeli, I’m mainly looking for code style review for this PR.

I’ve implemented the design and functionality that @mathjazz had requested, but am looking for code readability/performance feedback before I mark this PR as Open once again.

@harmitgoswami harmitgoswami marked this pull request as ready for review June 27, 2024 14:41
eemeli
eemeli previously requested changes Jul 1, 2024
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.

A bunch of small stuff, but the big issue is the inefficiency in how the data is queried from the DB.

pontoon/contributors/static/js/profile.js Outdated Show resolved Hide resolved
pontoon/contributors/static/js/profile.js Outdated Show resolved Hide resolved
pontoon/contributors/utils.py Outdated Show resolved Hide resolved
pontoon/contributors/utils.py Outdated Show resolved Hide resolved
pontoon/contributors/utils.py Outdated Show resolved Hide resolved
pontoon/contributors/views.py Outdated Show resolved Hide resolved
pontoon/contributors/utils.py Outdated Show resolved Hide resolved
@harmitgoswami harmitgoswami marked this pull request as draft July 4, 2024 20:29
Harmit Goswami and others added 19 commits July 11, 2024 10:24
TODO: Fix plurals for user-translations type, sort months chronologically, fix issue regarding "empty for some reason" dates
Refactored multiple data fields within the contributions object to restore the old timeline display. There still exists an bug regarding unrejected and unapproved translations not having an associated date. For the time being, I hardcode their dates to be the Unix Epoch, just so it clearly stands out.
Factored out redundant passes of sho
w_year variable in various functions. Modified relevant test cases to reflect the new change in return object structure.
</ul>

{% endfor %}
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: mind the blank line.

@harmitgoswami harmitgoswami marked this pull request as ready for review July 11, 2024 15:23
pontoon/contributors/static/js/profile.js Show resolved Hide resolved
pontoon/contributors/static/js/profile.js Show resolved Hide resolved
pontoon/insights/utils.py Show resolved Hide resolved
pontoon/contributors/static/css/profile.css Show resolved Hide resolved

if year_shown:
start = end - relativedelta(years=1, day=1)
timeline_title = "Contribution activity in the last year"
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we just hardcode this to "Contribution activity" in the template?

Then we'd need to always show month + year in the timeline - even for this month's data - which would result in a more consistent UX and simpler code with less IF-ELSE blocks.

{% endfor %}
{% else %}
{% for title, project_data in contribution_timeline.contributions.items() %}
{{ render_contribution_group(title, project_data.type, project_data.data.values() | list) }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That list at the end should be dropped - a more proper approach would be to set the default value to entries in render_contribution_group().

</div>
{% endfor %}
{% else %}
{% for title, project_data in contribution_timeline.contributions.items() %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it confusing that we have different "APIs" for this month's data and for the full year data. I would expect the backend functions to return data in the exact same format, but for different periods, so we wouldn't need to IF-ELSE here and in one or two other places.

@eemeli eemeli requested review from eemeli and removed request for eemeli July 15, 2024 19:19
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.

I tried removing myself as a reviewer, but that didn't seem to work. I'm fine with this if Matjaž is, but please explicitly re-request a review from me if one is desired.

@harmitgoswami harmitgoswami dismissed eemeli’s stale review July 16, 2024 16:16

Changes have been implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants