Skip to content

Conversation

@jodar
Copy link
Contributor

@jodar jodar commented Jan 26, 2018

Cas_users was changed to cas_people so that it would be given a more general name, and thus it would facilitate the insertion of people in the system, even if these people are not really a user of the system.


<tbody>
<% @people.each do |person| %>
<tr id="person-<%= person.id %>">
Copy link
Member

Choose a reason for hiding this comment

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

Indentação

</div>
</div>

<%= link_to "Novo Usuário", new_site_person_path(@site) if current_person.admin? %>
Copy link
Member

Choose a reason for hiding this comment

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

Nova pessoa

devise_for :users,
class_name: "Cas::User",
devise_for :people,
class_name: "Cas::Person",
Copy link
Member

Choose a reason for hiding this comment

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

Creio que o melhor seja

devise_for :users,
  class_name: "Cas::Person",

config/routes.rb Outdated
mount Shrine.presign_endpoint(:cache) => "/files/cache/presign"

authenticate :user, ->(u){ u.roles.include?('admin') } do
authenticate :person, ->(u){ u.roles.include?('admin') } do
Copy link
Member

Choose a reason for hiding this comment

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

:user

Copy link
Member

@kurko kurko left a comment

Choose a reason for hiding this comment

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

Ótimo PR. Deve ter dado bastante trabalho. Agora que ele está feito a gente começa a ver uns detalhes que não havíamos pensado antes, como o current_user. Pequenos ajustes faltando pra finalizar.

👏 👏 👏 👏

def set_current_user
@current_user = current_user
def set_current_person
@current_person = current_person
Copy link
Member

Choose a reason for hiding this comment

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

Para o usuário logado, o ideal é que a gente trate ele como user, portanto current_user seria mais adequado. Além disso, é o que a maioria dos devs usa como nomenclatura, então vai evitar confusão.

before_action :authenticate_user!
before_action :set_current_user
before_action :set_user_sites
before_action :authenticate_person!
Copy link
Member

Choose a reason for hiding this comment

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

Acho que mudando o route.rb, aqui fica authenticate_user!

@@ -1,5 +1,5 @@
class Cas::Api::FilesController < Cas::ApplicationController
skip_before_action :authenticate_user!
skip_before_action :authenticate_person!
Copy link
Member

Choose a reason for hiding this comment

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

Acho que mudando o route.rb, aqui fica authenticate_user!

def index
@activities = Cas::Activity
.where(site_id: current_user.sites.map(&:id))
.where(site_id: current_person.sites.map(&:id))
Copy link
Member

Choose a reason for hiding this comment

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

Acho que mudando o route.rb, aqui fica current_user

@@ -1,5 +1,5 @@
class Cas::FileUploadsController < Cas::ApplicationController
skip_before_action :authenticate_user!
skip_before_action :authenticate_person!
Copy link
Member

Choose a reason for hiding this comment

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

Acho que mudando o route.rb, aqui fica authenticate_user!


<% @sections.each do |section| %>
<% next unless Cas::SectionConfig.new(section).accessible_by_user?(current_user) %>
<% next unless Cas::SectionConfig.new(section).accessible_by_person?(current_person) %>
Copy link
Member

Choose a reason for hiding this comment

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

current_user

<%= csrf_meta_tags %>
</head>
<body class="<%= "site-#{@site.id}" if @current_user.present? && @site.present? %>">
<body class="<%= "site-#{@site.id}" if @current_person.present? && @site.present? %>">
Copy link
Member

Choose a reason for hiding this comment

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

current_user. Precisa revisar outros pontos aqui nesse arquivo também

rename_table :cas_sites_users, :cas_people_sites
rename_column :cas_people_sites, :user_id, :person_id
add_index :cas_people_sites, [:site_id, :person_id], using: "btree"
add_index :cas_activities, :person_id, using: "btree"
Copy link
Member

Choose a reason for hiding this comment

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

btree já é o padrão, não precisa estipular aqui. Pode omitir o using

@@ -0,0 +1,12 @@
class AlterCasUsersToCasPeople < ActiveRecord::Migration[5.0]
def change
Copy link
Member

Choose a reason for hiding this comment

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

👍


let!(:site1) { create(:site, name: "mysite1", slug: "mysite1") }
let!(:site2) { create(:site, name: "mysite2", slug: "mysite2") }
let!(:site3) { create(:site, name: "mysite3", slug: "mysite3") }
Copy link
Member

Choose a reason for hiding this comment

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

Um deles precisa ser mysite porque o fixtures/cas.yml tem um site chamado mysite. Olha acima,

let!(:site1) { create(:site, slug: "mysite1") }
let!(:site2) { create(:site, slug: "mysite") } # <====
let!(:site3) { create(:site, slug: "mysite3") }

belongs_to :author, class_name: "::Cas::User"
has_many :images, ->{ where(media_type: :image).order("cas_media_files.order ASC") }, class_name: "::Cas::MediaFile", as: :attachable, dependent: :destroy
belongs_to :author, class_name: "::Cas::Person"
has_many :images, ->{ where(media_type: :image).order("cas_media_files.order ASC") }, class_name: Cas::MediaFile, as: :attachable, dependent: :destroy
Copy link
Member

Choose a reason for hiding this comment

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

Faltou aqui


belongs_to :attachable, polymorphic: true
belongs_to :author, class_name: "::Cas::User"
belongs_to :author, class_name: "Cas::Person"
Copy link
Member

Choose a reason for hiding this comment

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

👆

class User < ApplicationRecord
ROLES = %w[admin editor writer].freeze
class Person < ApplicationRecord
ROLES = %w[admin editor writer visitor].freeze
Copy link
Member

Choose a reason for hiding this comment

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

☝️

has_many :sites_users, class_name: '::Cas::SitesUser'
has_many :sites, through: :sites_users
has_many :files, class_name: 'Cas::MediaFile', as: :attachable
has_many :people_site, class_name: 'Cas::PeopleSite'
Copy link
Member

Choose a reason for hiding this comment

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

☝️

has_many :sections, dependent: :destroy
has_many :sites_users, class_name: '::Cas::SitesUser'
has_many :users, through: :sites_users
has_many :people_site, class_name: '::Cas::PeopleSite'
Copy link
Member

Choose a reason for hiding this comment

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

☝️

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