[WIP] Only denormalize a build if it's the most recent #318
Conversation
| @@ -34,6 +35,15 @@ module States | |||
| # end | |||
| end | |||
|
|
|||
| def most_recent_build? | |||
| Build.order(created_at: :desc).first.id == id | |||
There was a problem hiding this comment.
@henrikhodne: Will this only find builds for the current repository? Is there a way to make this more efficient?
There was a problem hiding this comment.
No, this is a global query. Something like repository.builds.last would probably be better, although I'm not 100% sure if that's the most efficient query or correct.
|
@henrikhodne: Does this look good? |
|
I think this is correct and makes sense, but I'd like for someone else to look over this as well. |
|
@henrikhodne: Actually, I need to fix PR #319 first. |
|
@henrikhodne: I've fixed the other PR now. I think it's ready to merge. |
| @@ -194,6 +194,10 @@ def pull_request? | |||
| event_type == 'pull_request' | |||
| end | |||
|
|
|||
| def most_recent_build? | |||
| repository.builds.last.id == id | |||
There was a problem hiding this comment.
I'm not sure if this is right. This is the most recently created build, but it could be a PR build, it could be a branch build, etc. If we are to use this as some query for when to set the repository status, I think it should find the "latest build that is on the default branch and isn't a pull request build. Then it would look something like this, I think:
repository.builds.where(branch: repository.default_branch, event_type: "push").last.idIn that case, the method should probably be renamed too.
There was a problem hiding this comment.
@henrikhodne: I agree. I've updated the commit.
| @@ -194,6 +194,10 @@ def pull_request? | |||
| event_type == 'pull_request' | |||
| end | |||
|
|
|||
| def last_created_build? | |||
| repository.builds.where(branch: repository.default_branch, event_type: "push").last.id | |||
There was a problem hiding this comment.
This needs to be a comparison instead, right now it just fetches the last build's ID.
Also, I feel like this method is doing a bit much right now and that some of this should probably go in the repository model instead. Maybe Repository#last_build_on_default_branch and then check if this build is the same as that build?
There was a problem hiding this comment.
@henrikhodne: Good catch. I've changed it.
|
@Aaron1011 Awesome, thank you. I fixed the specs and merged this in 68c1088. |
| @@ -22,7 +22,7 @@ def denormalize(event, *args) | |||
| } | |||
|
|
|||
| def denormalize?(event) | |||
| DENORMALIZE.key?(event) | |||
| DENORMALIZE.key?(event) && last_build_on_default_branch? | |||
There was a problem hiding this comment.
@henrikhodne does this mean we only denormalize builds on the default branch? This is going to be confusing in the UI as a pusher event will come through for a PR build and update the side bar, but then if you refresh you won't see that update as the build was not denormalized.
There was a problem hiding this comment.
@joshk: Where does travis-web update the sidebar? Looking at the receive method, it doesn't seem to do much when a build:started event comes in.
|
I reverted this commit. See 183d8f1. |
|
So the denormalization logic is in travis-web as well? |
|
no, but it is partially tied to the UI. This has been the issue with our UI for a long time, and why we looked at changes which then became labs. |
|
How could we proceed here? There'd be changes required in travis-web as well? |
Don't merge this yet.