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

#4036 Updated build list to include an alert state #5222

Merged
merged 5 commits into from Feb 20, 2019

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Feb 3, 2019

This commit adds alert icon to build list if the build.state == 'triggered' and build.date > 5 mins ago.
This fixes #4036

@safwanrahman safwanrahman requested a review from agjohnson Feb 5, 2019
@safwanrahman safwanrahman requested a review from Feb 6, 2019
@humitos
Copy link
Member

@humitos humitos commented Feb 6, 2019

Thanks for the PR! Can you upload an screenshot, please?

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Feb 6, 2019

Thanks for the PR! Can you upload an screenshot, please?

Sure.
1

Copy link
Member

@humitos humitos left a comment

Thanks for your contribution!

I think this PR is close to get merged. I'd ask for @agjohnson to review it since it includes some CSS but also because he may give you some directions around the warning box.



@register.simple_tag
def get_past_time(mins):
Copy link
Member

@humitos humitos Feb 12, 2019

What about naming this as minutes_ago?

Copy link
Member Author

@saadmk11 saadmk11 Feb 12, 2019

I was confused about what to call it. Thanks for the suggestion. I will change it as soon as possible.

Copy link
Contributor

@agjohnson agjohnson left a comment

Thanks for the PR! For easier maintenance, we keep logic out of templates and put them on models instead, so this logic can be moved to the Build model. The rest of the PR looks 💯, but I'm sure will change a bit with the move of this logic.

@register.simple_tag
def minutes_ago(mins):
"""Subtracts provided minutes from current time."""
return timezone.now() - datetime.timedelta(minutes=int(mins))
Copy link
Contributor

@agjohnson agjohnson Feb 12, 2019

We tend to avoid putting logic in templates like this. It should probably be a property defined on the models. A property method like Build.is_stale would be useful outside of templates. We might eventually have to surface this in our API when we make this an API driven page, but for now, the build list page is tied to the database models directly.

Copy link
Member Author

@saadmk11 saadmk11 Feb 13, 2019

Thanks for the PR! For easier maintenance, we keep logic out of templates and put them on models instead, so this logic can be moved to the Build model. The rest of the PR looks , but I'm sure will change a bit with the move of this logic.

@agjohnson Thanks for your review. I have committed the changes you asked for. Please have a look and let me know if any further changes are required.

Copy link
Member

@humitos humitos left a comment

The new changes looks great! Thanks.

@@ -4,7 +4,7 @@
{% for build in build_qs %}
<li class="module-item col-span">
<div id="build-{{ build.id }}">
<a href="{{ build.get_absolute_url }}"><span id="build-state">{% if build.state != 'finished' %}{{ build.get_state_display }} {% else %} {% if build.success %}{% trans "Passed" %}{% else %}{% trans "Failed" %}{% endif %}{% endif %}</span>
<a href="{{ build.get_absolute_url }}">{% if build.is_stale %}<span class="icon-warning" title="{% trans 'This build is still waiting to be built' %}"></span>{% endif %}<span id="build-state">{% if build.state != 'finished' %}{{ build.get_state_display }} {% else %} {% if build.success %}{% trans "Passed" %}{% else %}{% trans "Failed" %}{% endif %}{% endif %}</span>
Copy link
Member Author

@saadmk11 saadmk11 Feb 20, 2019

@humitos What Do you think about changing {% if build.state != 'finished' %} to {% if build.finished %} as already there is a property method on Build model named Build.finished? as @agjohnson mentioned to avoid logic on templates.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 20, 2019

Changes look great, I like the test additions! Thanks for the PR and the update!

@agjohnson agjohnson merged commit 3218c80 into readthedocs:master Feb 20, 2019
1 check passed
@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Feb 20, 2019

The new changes looks great! Thanks.

Changes look great, I like the test additions! Thanks for the PR and the update!

Great. its my first contribution to open source! Hope to Continue.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 20, 2019

Great. its my first contribution to open source! Hope to Continue.

Woo, congratulations then! Definitely do continue!

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Feb 20, 2019

Great. its my first contribution to open source! Hope to Continue.

Woo, congratulations then! Definitely do continue!

Thanks 👍

@saadmk11 saadmk11 deleted the build-list-alert branch Feb 22, 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.

3 participants