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

adds video_viewers count model #66

Merged
merged 3 commits into from
Oct 14, 2020
Merged

adds video_viewers count model #66

merged 3 commits into from
Oct 14, 2020

Conversation

Sirbuland
Copy link
Collaborator

@Sirbuland Sirbuland commented Oct 14, 2020

This PR adds a model in database for keeping record of viewers of a video which is not live. We are also maintaining its count in video table in video_viwerers_count using counter_cache provided by active_record.

Copy link
Contributor

@HamptonMakes HamptonMakes left a comment

Choose a reason for hiding this comment

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

This is using an API call to register the view when it's not live.

I think it is a lot simpler if we just make this a part of the VideosController#show and have no client-side logic.

stream_from(stream_name)
end

def unsubscribed
current_video.update!(active_viewers: current_video.active_viewers - 1) if current_video.active_viewers
current_video.decrement_viewers! if current_video.active_viewers
stop_all_streams
Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually going to ask about this in the first PR, but forgot to mention

@@ -2,7 +2,6 @@

class ChatMessagesController < ApplicationController
before_action :authenticate_user!, only: [:create]
before_action :current_video
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

end
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather keep the whole string!

browser: browser_name,
}
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this! This should just be on the VideosController#show action! No API call or controller required!

@@ -90,6 +93,11 @@ export default class extends Controller {
);
}

incrementVideoViews(): void {
console.log(this.streamType);
if (this.streamType !== "live") post("./video_views");
Copy link
Contributor

Choose a reason for hiding this comment

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

Live Views count as Views! But also, same comment it should just be server side.

@@ -23,7 +23,7 @@ export default class ChatMessagesController extends BaseController {

private displayMessage(message) {
const sameUser = message.userId === this.lastMessageFromUserId;
console.log(message);

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops, that's my fault

before do
video.go_live!
end

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, you should be able to use the factory :live_video to get this.

video { nil }
details { "" }
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this? I don't think so.

video.reload
expect(video.video_views.size).to eq(1)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be in the VideosRequestSpec

@HamptonMakes
Copy link
Contributor

@Sirbuland Sirbuland force-pushed the VEUE-137 branch 2 times, most recently from bd71b5d to 2845b8b Compare October 14, 2020 17:26
Copy link
Contributor

@HamptonMakes HamptonMakes left a comment

Choose a reason for hiding this comment

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

LGTM!

@HamptonMakes HamptonMakes merged commit 6f5c0dc into main Oct 14, 2020
@HamptonMakes HamptonMakes deleted the VEUE-137 branch October 14, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants