-
Notifications
You must be signed in to change notification settings - Fork 1
Add migration alter cas_users to cas_people #2
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: master
Are you sure you want to change the base?
Conversation
|
|
||
| <tbody> | ||
| <% @people.each do |person| %> | ||
| <tr id="person-<%= person.id %>"> |
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.
Indentação
| </div> | ||
| </div> | ||
|
|
||
| <%= link_to "Novo Usuário", new_site_person_path(@site) if current_person.admin? %> |
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.
Nova pessoa
| devise_for :users, | ||
| class_name: "Cas::User", | ||
| devise_for :people, | ||
| class_name: "Cas::Person", |
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.
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 |
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.
:user
kurko
left a comment
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.
Ó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 |
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.
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! |
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.
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! | |||
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.
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)) |
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.
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! | |||
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.
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) %> |
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.
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? %>"> |
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.
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" |
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.
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 | |||
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.
👍
spec/acceptance/activities_spec.rb
Outdated
|
|
||
| let!(:site1) { create(:site, name: "mysite1", slug: "mysite1") } | ||
| let!(:site2) { create(:site, name: "mysite2", slug: "mysite2") } | ||
| let!(:site3) { create(:site, name: "mysite3", slug: "mysite3") } |
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.
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 |
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.
Faltou aqui
|
|
||
| belongs_to :attachable, polymorphic: true | ||
| belongs_to :author, class_name: "::Cas::User" | ||
| belongs_to :author, class_name: "Cas::Person" |
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.
👆
| class User < ApplicationRecord | ||
| ROLES = %w[admin editor writer].freeze | ||
| class Person < ApplicationRecord | ||
| ROLES = %w[admin editor writer visitor].freeze |
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.
☝️
| 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' |
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.
☝️
| 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' |
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.
☝️
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.