-
Notifications
You must be signed in to change notification settings - Fork 0
refactoring #75
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
base: develop
Are you sure you want to change the base?
refactoring #75
Changes from 36 commits
8bac5b0
9685bc3
c63eb8b
eab445d
f4578e3
b8703d4
f443726
30a66ca
0a4d12c
c1be315
a906740
503a783
b15f867
ce66ae2
17b17d3
1e770b0
d419bcb
0f93138
ee7e900
0bb6c22
03e7102
34544af
26f80da
6e93ee0
fb05e39
6f011a7
0c55f34
5f7824b
ae05494
6bf4edf
2c5342f
058b238
37efbda
9f9d99c
a8106b7
3b8b882
b7c4da4
432274d
6eed13c
ff31f54
83f6818
6206f9a
dd56a25
0293f8a
483fc73
0c33b29
01f5de8
014230d
6c6f70e
dc3db55
8984936
2496747
d174db3
f241a91
ba5eec0
b719e16
0371c6b
1290b14
e2d3948
585f57c
15612e2
4f3dfe8
91d0fd9
c6a32e8
88f3d63
ecf5719
08bc5e4
3e8540e
52f176e
a6a1c91
78a4d85
b67c214
1b0599e
4f743c0
7e7f7f8
b429d5a
d4b5ea9
ad41d0e
9b2f84c
a670aa2
9db7725
f1deb15
8485ce0
372a718
98d4da9
be4de17
994534e
b920d12
68bd19a
310e49b
f2bfc57
ab8dc1e
6a66a0c
4d1895a
bdd4890
ea00633
bef894f
2b94b97
6e79e22
c3a9139
2b1225a
7565948
0f8f132
62f5f9e
45b855e
30e9daa
23a5259
13c8925
e6a7369
f9f9869
1fee544
ab8f4c7
9a0677d
ac69b3f
d0fe754
405a2f6
fe6bb12
23102af
0baa2e3
e74bf13
f4b6b42
d369859
6e124f0
74551ce
6db77ce
f19dcd3
a4fffae
58d64af
db3f218
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,7 @@ class TournamentsController < ApplicationController | |||||
| before_action :authenticate_user!, except: [:index] | ||||||
|
|
||||||
| def index | ||||||
| @tournaments = Tournament.includes(:users).all | ||||||
| @tournaments = Tournament.includes(%i[users winner]).all | ||||||
| @tournaments = @tournaments.order(:registration_deadline).page(params[:page]) | ||||||
| end | ||||||
|
|
||||||
|
|
@@ -28,10 +28,10 @@ def register | |||||
| def create_matches | ||||||
| registered_users = @tournament.users | ||||||
| length = registered_users.length | ||||||
| return unless (length - 8) != 0 | ||||||
| return if (length - 8) != 0 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Check if length is power of 2 or not. |
||||||
|
|
||||||
| begin | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| MatchCreator.new(@tournament, registered_users).create_match | ||||||
| MatchCreator.new(@tournament, registered_users).call | ||||||
| redirect_to tournament_path(@tournament), notice: 'Matches Generated' | ||||||
| rescue StandardError => e | ||||||
| redirect_to tournament_path(@tournament), alert: "Error generating matches: #{e.message}" | ||||||
|
|
@@ -40,7 +40,6 @@ def create_matches | |||||
|
|
||||||
| def create | ||||||
| @tournament = Tournament.new(tournament_params) | ||||||
|
|
||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Load everything using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try to show why an operation failed so the user can take appropriate actions, -> https://github.com/techiealiraza/rock-papper-sissors-app/pull/75/files#diff-c9c5ff5ae897df4b0504a76d7549d657c8b3c20875611584db7552f5a83ac229L48 |
||||||
| respond_to do |format| | ||||||
| if @tournament.save | ||||||
| format.html { redirect_to tournament_url(@tournament), notice: 'Tournament was successfully created.' } | ||||||
|
|
@@ -77,7 +76,7 @@ def tournament_params | |||||
| :description, | ||||||
| :start_date, | ||||||
| :end_date, | ||||||
| :tournament_winner_id, | ||||||
| :winner_id, | ||||||
| :image, | ||||||
| :registration_deadline) | ||||||
| end | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,43 +1,29 @@ | ||||||
| # frozen_string_literal: true | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed ? |
||||||
|
|
||||||
| module Users | ||||||
| class SessionsController < Devise::SessionsController | ||||||
| before_action :authenticate_2fa!, only: [:create] | ||||||
| before_action :otp_generate_and_send, only: [:create] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| before_action :authenticate_user!, except: %i[new create destroy] | ||||||
| before_action :load_and_authorize_resource, except: %i[new create destroy] | ||||||
|
|
||||||
| def create | ||||||
| super do |resource| | ||||||
| if resource.valid? && resource.persisted? | ||||||
| resource.update( | ||||||
| otp_required_for_login: true | ||||||
| ) | ||||||
| end | ||||||
| end | ||||||
| def verify_otp | ||||||
| authenticate_2fa! | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are calling just another method from the main method ? Never do it. Always try to break the function and call multiple functions from there ? |
||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| def authenticate_2fa! | ||||||
| user = self.resource = find_user | ||||||
| return unless user | ||||||
|
|
||||||
| if otp_attempt_present? | ||||||
| authenticate_with_2fa(user) | ||||||
| sign_in(:user, user) | ||||||
| elsif valid_password_and_otp_required?(user) | ||||||
| session[:user_id] = user.id | ||||||
| send_otp_code(user) | ||||||
| render 'user_otp/two_fa' | ||||||
| end | ||||||
| user = User.find_by(id: session[:user_id]) | ||||||
| User.auth_with_2fa(user_params[:otp_attempt], user) | ||||||
| sign_in(:user, user) | ||||||
| redirect_to root_path, notice: 'OTP consumed Successfully.' | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User can login without entering password and OTP, |
||||||
| end | ||||||
|
|
||||||
| def otp_attempt_present? | ||||||
| user_params[:otp_attempt].present? | ||||||
| end | ||||||
| def otp_generate_and_send | ||||||
| user = self.resource = User.find_by(email: user_params[:email]) | ||||||
|
|
||||||
| def authenticate_with_2fa(user) | ||||||
| User.auth_with_2fa(user_params[:otp_attempt], user) | ||||||
| return unless user | ||||||
|
|
||||||
| valid_password_and_otp_required?(user) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will be sending OTP even if the password is wrong. This is a security risk. |
||||||
| session[:user_id] = user.id | ||||||
| send_otp_code(user) | ||||||
| render 'user_otp/two_fa' | ||||||
| end | ||||||
|
|
||||||
| def valid_password_and_otp_required?(user) | ||||||
|
|
@@ -48,14 +34,6 @@ def send_otp_code(user) | |||||
| TwoFactorAuth.new(user).send_otp_code | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed about convention of using |
||||||
| end | ||||||
|
|
||||||
| def find_user | ||||||
| if session[:user_id] | ||||||
| User.find_by(id: session[:user_id]) | ||||||
| elsif user_params[:email] | ||||||
| User.find_by(email: user_params[:email]) | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| def user_params | ||||||
| params.require(:user).permit(:authenticity_token, :email, :password, :otp_attempt, :remember_me) | ||||||
| end | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| module ApplicationHelper | ||
| def custom_index(collection, index) | ||
| per_page = collection.limit_value | ||
| starting_index = (collection.current_page - 1) * per_page + 1 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| starting_index + index | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,41 +13,38 @@ document.addEventListener("turbolinks:load", () => { | |||||
| received(data) { | ||||||
| const msgs = document.getElementById("message-list"); | ||||||
| const current_user = document.getElementById("current_user"); | ||||||
| const current_user_name = current_user.getAttribute( | ||||||
| "data-current-user-name" | ||||||
| ); | ||||||
| const msg_element = document.getElementById( | ||||||
| `msg_field_${data.user_name}` | ||||||
| ); | ||||||
| const current_user_name = current_user.getAttribute("data-current-user-name"); | ||||||
| const msg_element = document.getElementById(`msg_field_${data.user_name}`); | ||||||
| let messageHTML; | ||||||
| if (data.user_name === current_user_name) { | ||||||
| messageHTML = `<div class="my-1 flex flex-row"> | ||||||
| <div class="rounded-full bg-white w-7 text-black text-center h-7"> | ||||||
| ${data.user_name.substring(0, 3)} | ||||||
| </div> | ||||||
| <div class ="flex flex-col space-y-2 text-xs ml-2 items-start"> | ||||||
| <div class="line-clamp-10 max-w-sm text-lg px-3 py-2 my-1 rounded-lg rounded-br-none rounded-tl-none bg-white text-black"> | ||||||
| ${data.message} | ||||||
| </div> | ||||||
| <div class="flex flex-row items-end"> | ||||||
| <div class="text-white">${data.created_at}</div> | ||||||
| </div> | ||||||
| </div> | ||||||
| </div>`; | ||||||
| } else { | ||||||
| messageHTML = `<div class="my-1 flex flex-row"> | ||||||
| <div class="rounded-full bg-gold_shade2 w-7 text-white text-center h-7"> | ||||||
| ${data.user_name.substring(0, 3)} | ||||||
| </div> | ||||||
| <div class="flex flex-col space-y-2 text-xs ml-2 items-start"> | ||||||
| <div class="line-clamp-10 max-w-sm text-lg px-3 py-2 my-1 rounded-lg rounded-br-none rounded-tl-none bg-gold text-black"> | ||||||
| ${data.message} | ||||||
| </div> | ||||||
| <div class="flex flex-row items-end"> | ||||||
| <div class="text-white">${data.created_at}</div> | ||||||
| </div> | ||||||
| </div> | ||||||
| </div>`; | ||||||
| messageHTML = `<div class = "my-1 flex flex-row"> | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| <div class = "rounded-full bg-white w-7 text-black text-center h-7"> | ||||||
| ${data.user_name.substring(0, 3)} | ||||||
| </div> | ||||||
| <div class ="flex flex-col space-y-2 text-xs ml-2 items-start"> | ||||||
| <div class="line-clamp-10 max-w-sm text-lg px-3 py-2 my-1 rounded-lg rounded-br-none rounded-tl-none bg-white text-black"> | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spacing issue |
||||||
| ${data.message} | ||||||
| </div> | ||||||
| <div class = "flex flex-row items-end"> | ||||||
| <div class = "text-white">${data.created_at}</div> | ||||||
| </div> | ||||||
| </div> | ||||||
| </div>`; | ||||||
| } | ||||||
| else { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for if else, DRY this code. |
||||||
| messageHTML = `<div class = "my-1 flex flex-row"> | ||||||
| <div class = "rounded-full bg-gold_shade2 w-7 text-white text-center h-7"> | ||||||
| ${data.user_name.substring(0, 3)} | ||||||
| </div> | ||||||
| <div class ="flex flex-col space-y-2 text-xs ml-2 items-start"> | ||||||
| <div class="line-clamp-10 max-w-sm text-lg px-3 py-2 my-1 rounded-lg rounded-br-none rounded-tl-none bg-gold text-black"> | ||||||
| ${data.message} | ||||||
| </div> | ||||||
| <div class = "flex flex-row items-end"> | ||||||
| <div class = "text-white">${data.created_at}</div> | ||||||
| </div> | ||||||
| </div> | ||||||
| </div>`; | ||||||
| } | ||||||
|
|
||||||
| msgs.insertAdjacentHTML("afterbegin", messageHTML); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,24 +3,28 @@ import { Controller } from "@hotwired/stimulus"; | |
| export default class extends Controller { | ||
| connect = (format = "seconds") => { | ||
| const reg_date = this.data.get("time"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for this variable |
||
| const tournament_id = this.data.get("id"); | ||
| const number = reg_date; | ||
| const time_in_seconds = reg_date; | ||
| const daysElement = document.getElementById("days"); | ||
| const hoursElement = document.getElementById("hours"); | ||
| const minutesElement = document.getElementById("minutes"); | ||
| const secondsElement = document.getElementById("seconds"); | ||
| let countdown; | ||
| console.log(time_in_seconds); | ||
| if (time_in_seconds < 0) { | ||
| clearDivs(); | ||
| return; | ||
| } | ||
| convertFormat(format); | ||
| function convertFormat(format) { | ||
| switch (format) { | ||
| case "seconds": | ||
| return timer(number); | ||
| return timer(time_in_seconds); | ||
| case "minutes": | ||
| return timer(number * 60); | ||
| return timer(time_in_seconds * 60); | ||
| case "hours": | ||
| return timer(number * 60 * 60); | ||
| return timer(time_in_seconds * 60 * 60); | ||
| case "days": | ||
| return timer(number * 60 * 60 * 24); | ||
| return timer(time_in_seconds * 60 * 60 * 24); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for this function. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -32,18 +36,21 @@ export default class extends Controller { | |
| const secondsLeft = Math.round((then - Date.now()) / 1000); | ||
|
|
||
| if (secondsLeft <= 0) { | ||
| document.getElementById("days").style.display = "none"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| document.getElementById("hours").style.display = "none"; | ||
| document.getElementById("minutes").style.display = "none"; | ||
| document.getElementById("seconds").style.display = "none"; | ||
| clearDivs(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed |
||
| clearInterval(countdown); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe its not needed. |
||
| location.reload; | ||
| return; | ||
| } | ||
|
|
||
| displayTimeLeft(secondsLeft); | ||
| }, 1000); | ||
| } | ||
|
|
||
| function clearDivs() { | ||
| daysElement.style.display = "none"; | ||
| hoursElement.style.display = "none"; | ||
| minutesElement.style.display = "none"; | ||
| secondsElement.style.display = "none"; | ||
| } | ||
| function displayTimeLeft(seconds) { | ||
| daysElement.textContent = Math.floor(seconds / 86400); | ||
| hoursElement.textContent = Math.floor((seconds % 86400) / 3600); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,14 +9,8 @@ function event_listener_to_buttons() { | |
| var user_image; | ||
| var flag = false; | ||
|
|
||
| if (player1_id === user_id) { | ||
| user_image = document.getElementById(player1_id); | ||
| flag = true; | ||
| } else if (player2_id === user_id) { | ||
| user_image = document.getElementById(player2_id); | ||
| flag = true; | ||
| } | ||
| if (flag) { | ||
| if(player1_id === user_id || player2_id === user_id){ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [player1_id, player2_id].include(user_id) No samosa point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/techiealiraza/rock-papper-sissors-app/pull/75/files#diff-7e7316016254761f0ee381db21b216342ff6970f53f9ab9e96205211a2533bc5R39 use this method instead of line 29 - 31 |
||
| user_image = document.getElementById(user_id); | ||
| document | ||
| .getElementById("rock_button") | ||
| .addEventListener("click", function () { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed.
Already commented here -> https://github.com/techiealiraza/rock-papper-sissors-app/pull/69/files#r1214302158