Remove state from profile #435
Conversation
m-an
force-pushed
the
feature/remove_state_from_profile
branch
3 times, most recently
from
5040816
to
c77ad31
Compare
m-an
force-pushed
the
feature/remove_state_from_profile
branch
2 times, most recently
from
22382e8
to
157cdc0
Compare
|
|
||
| private | ||
|
|
||
| def create_or_update_document_label |
There was a problem hiding this comment.
I can't understand why do you duplicate label logic in model and controller?
There was a problem hiding this comment.
here we add label 'document:verified' on document upload if there's no label 'document:verified'
There was a problem hiding this comment.
it is not duplicating any logic,
correct me, please, if I'm wrong
| def send_document_review_notification | ||
| if value == 'verified' | ||
| ProfileReviewMailer.approved(account).deliver_now | ||
| EventAPI.notify('system.document.verified', account_uid: account.uid) |
There was a problem hiding this comment.
add TODO: Need to update specs/event_api.md if you don't add it in current request. Specs are important for users of Events API
app/models/phone.rb
Outdated
| @@ -4,7 +4,7 @@ | |||
| # Class Phone | |||
| # | |||
| class Phone < ApplicationRecord | |||
| belongs_to :account | |||
| belongs_to :account, optional: true | |||
There was a problem hiding this comment.
oh, sorry, accidentaly added it from another branch
| <div class="dropdown show"> | ||
| <a class="btn btn-secondary dropdown-toggle" href="#" role="button" id="dropdownMenuLink" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"> | ||
| <%= @profile.state.upcase_first %> | ||
| <%= @profile.account.labels.find_by(key: 'document', scope: 'private')&.value %> |
There was a problem hiding this comment.
Never do any ActiveRecord find, where in a view
| <% Profile::STATES.each do |state|%> | ||
| <%= link_to(state.upcase_first, change_state_admin_profile_path(@profile, state: state), method: :put, class: 'dropdown-item') unless @profile.state == state%> | ||
| <% Document::STATES.each do |state|%> | ||
| <%= link_to(state.upcase_first, document_label_admin_profile_path(@profile, state: state), method: :put, class: 'dropdown-item') unless @profile.account.labels.find_by(key: :document).value == state%> |
There was a problem hiding this comment.
same as above. And use short string. It's hard to read
spec/models/document_spec.rb
Outdated
|
|
||
| it 'does not add new label' do | ||
| create_document | ||
| expect(current_account.labels.count).to eq 1 |
spec/models/document_spec.rb
Outdated
|
|
||
| it 'remains value verified' do | ||
| create_document | ||
| expect(current_account.labels.first.value).to eq 'verified' |
spec/models/label_spec.rb
Outdated
|
|
||
| context 'document label changes' do | ||
| let!(:account) { create(:account) } | ||
| let(:mailer_deliveries) { ActionMailer::Base.deliveries } |
There was a problem hiding this comment.
why not let(:last_mailer_delivery) { ActionMailer::Base.deliveries.last }
spec/models/label_spec.rb
Outdated
|
|
||
| context 'can be created with different scopes' do | ||
| it 'account has both labels' do | ||
| expect(account.labels.first).to eq label_public |
There was a problem hiding this comment.
expect(account.labels).to eq [label_public, label_private]
spec/models/label_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
| context 'with public labels duplicating levels' do |
There was a problem hiding this comment.
Wrong name. It's not implicit reason for spec.
I suggest when account has all required label for level 4 but in public scope
m-an
force-pushed
the
feature/remove_state_from_profile
branch
from
157cdc0
to
404aeb3
Compare
rxx
force-pushed
the
feature/remove_state_from_profile
branch
from
031d752
to
32eab69
Compare
|
already merged with #460 branch |
closes #445
closes #443
closes #442