gocd / gocd

GoCD - Continuous Delivery server main repository
https://www.gocd.org
Apache License 2.0
7.05k stars 973 forks source link

Move away from 'x minutes/days/months/years ago' time format #3323

Closed ibnc closed 7 years ago

ibnc commented 7 years ago

Specifically, moving to a format that includes the exact date and time a pipeline/job was triggered.

Job history:

job_history_sidebar

I'm not 100% sure what the format should be for the pipelines page. I have 2 ideas:

Option 1 (what's in this pr):

option_1

Option 2:

option_2

This PR doesn't contain any changes to the pipeline instance page. I'm still not 100% sure what to do about that page, but I wanted to get feedback on what I've got so far. What do you think @arvindsv @naveenbhaskar

Pipeline instance page:

screen shot 2017-03-27 at 4 16 24 pm
naveenbhaskar commented 7 years ago

I prefer option one. in option 2 "triggered" is repeating which clutters the section

arvindsv commented 7 years ago

I don't mind the combined option too. But, the "about X hours ago" bit on both pages is a bit of a lie. I believe it shows the time of the latest commit or something like that. It might be useful to change that to be accurate.

arvindsv commented 7 years ago

Don't know if it makes sense to hide the year if it's the same as the current one.

ibnc commented 7 years ago

@arvindsv @naveenbhaskar I'll go with option 1 then :)

@arvindsv: You are correct in saying that the "x hours ago" bit on the pipeline history page is how long ago the material was updated.

But you're incorrect in regards to the dashboard. The "x hours ago" displayed on the dashboard for each pipeline is actually when the pipeline was scheduled.

Honestly, I thought they both indicated the same thing until you said something about it, and then I took a look. My instinct is to leave the pipeline history page alone, and just go ahead with the modifications to the dashboard and job history sidebar. Thoughts?

arvindsv commented 7 years ago

Ok, I agree.

arvindsv commented 7 years ago

Wait. Are you saying the pipeline history page will still say "14 hours ago"? I know the time is wrong, but should it really be inconsistent - compared to the dashboard? I'd think not. What do you think?

arvindsv commented 7 years ago

Ah, I see this:

This PR doesn't contain any changes to the pipeline instance page. I'm still not 100% sure what to do about that page, but I wanted to get feedback on what I've got so far.

Alright.

ibnc commented 7 years ago

@arvindsv Yeah, it will still say the old "x hours ago" format.

The pipeline history time currently indicates when the material was updated, but it's not super clear just by looking at it. I've been using Go for a few years now, and I've always assumed that the time info on the dashboard and pipeline history page were indicating the same thing. I only now know that the pipeline history time info is about the materials because I looked at the code.

I guess I'm unclear what you mean the time is wrong on the pipeline history page. How is it wrong?

arvindsv commented 7 years ago

Oh, just wrong in the way you mentioned. In that it's not the trigger time.

ibnc commented 7 years ago

@arvindsv Should I go ahead and change the pipeline history time to be the same as the dashboard?

arvindsv commented 7 years ago

Yeah, I don't see why it should be the different from the others. As you know, that page might completely change soon. So, don't spend too much time on it. If it's a small change, I'd just do it.

ibnc commented 7 years ago

@arvindsv It wasn't too complicated. How does this look?

pipeline_history

I had to remove the timezone info for the pipeline history page to keep everything on one line. idk if that's a big deal or not

ibnc commented 7 years ago

@arvindsv I went ahead and pushed it up. Let me know if anything needs to change on how I'm displaying the date on the pipeline history page

ibnc commented 7 years ago

@jyotisingh @ketan @arvindsv After these changes go in, there won't be anyplace in the code that uses the jquery lib timeago. Am I good to remove that library?

arvindsv commented 7 years ago

@ibnc

I had to remove the timezone info for the pipeline history page to keep everything on one line. idk if that's a big deal or not

I think it's ok. But, there's always so much space on the right there, that I think we should use it better and make the first column wider. I'll leave it to @naveenbhaskar and you.

jyotisingh commented 7 years ago

@ibnc - all usages of that library would be gone with this PR, so we should be good to remove jQuery.timeago.

Just ran through the changes made in the PR, looks good to me. Few things though:

ibnc commented 7 years ago

@jyotisingh

I'm just starting my day, and will look into the failing tests.

On the date format. I'm not attached to it really. It just read more colloquially to me, but that may not apply to folks in different countries. I'll switch back for ease of testing.

ibnc commented 7 years ago

@jyotisingh I've pushed a fix for the broken tests, and switched back to the previous date format. I've removed jquery.timeago, and will push it once the PR build passed.

I do have a question on one of your bullet points though. What specifically do you mean by the "Need a functional tests PR for updating the assertions for the scheduled times." bullet point?

arvindsv commented 7 years ago

I guess things like this? Not sure whether date is being verified though.

gocd-cla-bot commented 7 years ago

CLA assistant check
All committers have signed the CLA.

ibnc commented 7 years ago

@ketan Changes made. Let me know if everything looks good

ibnc commented 7 years ago

@ketan @arvindsv @jyotisingh @naveenbhaskar Do y'all think this PR is ready to merge, or is there some work that I still need to do?

arvindsv commented 7 years ago

I'm fine with it, the way it is. I've only seen the screenshots, though. Haven't tried it or looked at the code.

ketan commented 7 years ago

I'll merge it today.