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

Optimizations and UX improvements to the dashboard screen #5637

Merged
merged 5 commits into from May 30, 2019

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Apr 27, 2019

  • No longer show how many builds a project has. This is expensive and not very useful
  • Instead, show the date of the last build
  • Fixes a performance issue affecting the corporate site

Note that I used the HTML5 <time> tag to represent the datetime. This tag has attributes that have a machine readable datetime as well as a title with a precise locale-specific human-readable datetime as opposed to the nebulous "a year ago". I think we should use this convention or something like it more widely.

This PR builds on some optimizations in #5472 and this should be refactored slightly if that is merged first.

Screenshot

Screen Shot 2019-04-26 at 8 58 09 PM

- No longer show how many builds a project has
  This is expensive and not very useful
- Instead, show the date of the last build
- Fixes a performance issue affecting the corporate site
@davidfischer davidfischer requested a review from Apr 27, 2019
@davidfischer davidfischer changed the title Optimizations and UX improvements to the dashboard Optimizations and UX improvements to the dashboard screen Apr 27, 2019
humitos
humitos approved these changes May 6, 2019
Copy link
Member

@humitos humitos left a comment

Looks good to me!

Although I left some comments, feel free to merge ;)


# Prefetch the latest build for each project.
subquery = Subquery(
Build.objects.filter(project=OuterRef('project_id')).values_list('id', flat=True)[:1]
Copy link
Member

@humitos humitos May 6, 2019

I think it's not needed because ordering = ['-date'] in the model, but just in case: don't we need to order_by here?

Copy link
Contributor Author

@davidfischer davidfischer May 9, 2019

It isn't necessary due to the default ordering but maybe it is best to be explicit.

@@ -845,6 +845,12 @@ def get_latest_build(self, finished=True):
:param finished: Return only builds that are in a finished state
"""
if hasattr(self, '_latest_build'):
# Cached latest build
Copy link
Member

@humitos humitos May 6, 2019

I think it could be useful to extend this comment indicating where this comes from (dashboard queryset), why it's needed or similar since the source it's in another file and I think we will not find this quickly when reading the code later.

Copy link
Contributor Author

@davidfischer davidfischer May 9, 2019

I agree. How about after #5472 is merged I'll refactor this slightly to have a class variable for caching it.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 10, 2019

I refactored this optimization to be on the manager instead of sprinkled in a few functions.

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented May 10, 2019

Seems like a better approach :)

Copy link
Member

@humitos humitos left a comment

💯

@davidfischer davidfischer merged commit 678d360 into master May 30, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the davidfischer/dashboard-latest-build-date branch May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants