Improve registrant area #932

artur-intech · 2018-08-12T22:44:00Z

Improved UI
Contacts are now nested under domains
Tests added

Part of #849

maciej-szlosarczyk · 2018-08-13T19:04:59Z

app/controllers/registrant/contacts_controller.rb

@@ -26,4 +27,8 @@ def domain_ids
      BusinessRegistryCache.fetch_by_ident_and_cc(ident, ident_cc).associated_domain_ids
    end
  end
+
+  def domain
+    Domain.find(params[:domain_id])


This runs a database query every time you call domain. You probably want:

def domain @domain ||= Domain.find(params[:domain_id]) end

Not necessarily true https://guides.rubyonrails.org/caching_with_rails.html#sql-caching.

In general, I agree that ||= is better, but I think it is still a premature optimization, and it's not worth paying too much attention to this.

There's an additional problem with that, at the moment it's visible only in the breadcrumb, but it might balloon to something worse.

Go to any registrant's contact show page.

In URL, in id field enter an id from an existing domain that does not belong to the same Registrant.

Observe the breadcrumb - it will have a domain name from a domain that does not belong to this contact or the same user.

Good catch! Not sure if it's worth the effort, since I had to copy-paste some chunk that needs refactoring.

Anyway, it is now fixed. The bug is quite nasty, that's for sure.

maciej-szlosarczyk · 2018-08-13T19:05:55Z

app/views/registrant/contacts/show.html.erb

+<ol class="breadcrumb">
+    <li><%= link_to t('registrant.domains.index.header'), registrant_domains_path %></li>
+    <li><%= link_to domain, registrant_domain_path(domain) %></li>
+    <li><%= t 'registrant.contacts.contact_index' %></li>


Please stick to one way of calling translations helpers inside the same file, either with or without brackets.

What do you mean? I can't omit t() along with link_to

Then use t('registrant.contacts.contact_index') here too.

Why I should? I consider omitting brackets when they are not needed a good practice.

Let's assume that it's an internal DSL, and leave it be, but generally, that's not a good practice:
https://github.com/rubocop-hq/ruby-style-guide#method-invocation-parens

I don't think it is DSL. I just find it a bit more clear to omit braces in this particular case.

maciej-szlosarczyk · 2018-08-13T19:08:04Z

config/routes.rb

    resources :domains, only: %i[index show] do
+      resources :contacts, only: %i[show]


Just a symbol works too, no need for an Array of one element.

Another branch already adds other actions, so it will anyway become an array

maciej-szlosarczyk · 2018-08-13T19:16:41Z

app/views/registrant/domains/partials/_contacts.html.erb

+                <% contacts.each do |contact| %>
+                    <tr>
+                        <td>
+                            <%= link_to contact, registrant_domain_contact_path(domain, contact) %>


This is error prone, it depends on us overwriting to_s method for Contact. It would be better if this would be made explicit (contact.name)

https://github.com/internetee/registry/blob/master/app/models/contact.rb#L284-L286

Why providing a custom implementation of to_s for Contact class is a bad thing and what exactly is prone to error given we have tests for that part?

It is prone to error because as being implicit, it relies on undocumented behaviour.
None of this is publicly documented, neither in examples for link_to or not in anything that goes deeper than that.
This is where this comes from: https://github.com/rails/rails/blob/v4.2.9/actionview/lib/action_view/helpers/tag_helper.rb#L146

In Rails 5.2, that is changed quite a lot: rails/rails@a65a3bd#diff-2eb399a01976bca6aad9e85aa67b403d

On top of everything, none of the actual tag helper tests test for this scenario (passing an object instead of a string to a tag. https://github.com/rails/rails/blob/master/actionview/test/template/tag_helper_test.rb

Where's the test for this link? Or do you mean the fact that we do have a test for Contact#to_s?

maciej-szlosarczyk

It would be also really good if you'd add tests for the domain/show action and for the new contact/show action. They're missing.

maciej-szlosarczyk · 2018-08-16T06:34:15Z

You still have the bug I pointed out. Here's a simple test case against which you can fix it, goes into RegistrantAreaContactDetailsTest.

  # Domain metro belongs to jack, not john. It also does not have any contacts.
  def test_domain_from_different_registrar_does_not_leak_in_view
    visit registrant_domain_contact_url(domains(:metro), @contact)
    refute_text('metro.test')
  end

vohmar · 2018-08-16T16:13:12Z

Logic of accessing registrant contact data has changed - why? This is not good as throuout the application everywhere user accesses contact data by clicking on the name of the contact (look at admin and tech contacts and registrar records). This change is confusing.

artur-intech · 2018-08-16T19:08:42Z

From my point of view it's just the opposite. The link explicitly tells a user that he can click on it to see the details of a registrant. As a shortcut, I might revert old behavior as well, if it works for you.

# Conflicts: # test/fixtures/contacts.yml

vohmar · 2018-09-17T08:33:13Z

accessing registrant details is still not fixed.

artur-intech · 2018-09-17T08:39:30Z

@vohmar See my question above.

artur-intech · 2018-09-18T11:16:51Z

UI is reverted to the previous state.

Improve registrant area

638167e

artur-intech assigned vohmar Aug 13, 2018

Remove translation

a2d3377

maciej-szlosarczyk reviewed Aug 13, 2018

View reviewed changes

maciej-szlosarczyk suggested changes Aug 14, 2018

View reviewed changes

vohmar assigned artur-intech and unassigned vohmar Aug 14, 2018

Artur Beljajev added 3 commits August 14, 2018 21:35

Merge branch 'master' into improve-registrant-area

e334fd9

Refactor contact list

ca5edb1

Add registrant area tests

5a46620

artur-intech force-pushed the improve-registrant-area branch from f5242bc to 5a46620 Compare August 15, 2018 09:58

Do not show contact if given domain does not belong to the current user

c89cce6

artur-intech assigned vohmar and unassigned artur-intech Aug 16, 2018

maciej-szlosarczyk approved these changes Aug 16, 2018

View reviewed changes

vohmar assigned artur-intech and unassigned vohmar Aug 16, 2018

Artur Beljajev added 6 commits August 17, 2018 22:19

Merge branch 'master' into improve-registrant-area

eeec8dd

Remove unused and buggy code

8f0f236

Merge branch 'master' into improve-registrant-area

d274432

Integrate master

034e8e9

Merge branch 'master' into improve-registrant-area

c1e60f6

Merge branch 'master' into improve-registrant-area

3ca9536

# Conflicts: # test/fixtures/contacts.yml

artur-intech mentioned this pull request Sep 1, 2018

Registry 849 #958

Merged

artur-intech assigned vohmar and unassigned artur-intech Sep 15, 2018

vohmar assigned artur-intech and unassigned vohmar Sep 17, 2018

Artur Beljajev added 4 commits September 18, 2018 13:04

Merge branch 'master' into improve-registrant-area

1b16c92

Restore old UI

d9526c6

Use default translation

cabf922

Fix test

e4db808

artur-intech force-pushed the improve-registrant-area branch from f0a56e8 to e4db808 Compare September 18, 2018 11:03

artur-intech assigned vohmar and unassigned artur-intech Sep 18, 2018

vohmar merged commit 64d8be0 into master Sep 19, 2018

vohmar deleted the improve-registrant-area branch October 3, 2018 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve registrant area #932

Improve registrant area #932

artur-intech commented Aug 12, 2018 •

edited

Loading

maciej-szlosarczyk Aug 13, 2018

artur-intech Aug 14, 2018

maciej-szlosarczyk Aug 14, 2018 •

edited

Loading

artur-intech Aug 16, 2018 •

edited

Loading

maciej-szlosarczyk Aug 13, 2018

artur-intech Aug 14, 2018

maciej-szlosarczyk Aug 14, 2018

artur-intech Aug 16, 2018

maciej-szlosarczyk Aug 16, 2018

artur-intech Aug 17, 2018

maciej-szlosarczyk Aug 13, 2018

artur-intech Aug 14, 2018

maciej-szlosarczyk Aug 13, 2018

artur-intech Aug 14, 2018

maciej-szlosarczyk Aug 14, 2018 •

edited

Loading

maciej-szlosarczyk left a comment

maciej-szlosarczyk commented Aug 16, 2018

vohmar commented Aug 16, 2018

artur-intech commented Aug 16, 2018 •

edited

Loading

vohmar commented Sep 17, 2018

artur-intech commented Sep 17, 2018

artur-intech commented Sep 18, 2018

		resources :domains, only: %i[index show] do
		resources :contacts, only: %i[show]

Improve registrant area #932

Improve registrant area #932

Conversation

artur-intech commented Aug 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciej-szlosarczyk Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

artur-intech Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciej-szlosarczyk Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

maciej-szlosarczyk left a comment

Choose a reason for hiding this comment

maciej-szlosarczyk commented Aug 16, 2018

vohmar commented Aug 16, 2018

artur-intech commented Aug 16, 2018 • edited Loading

vohmar commented Sep 17, 2018

artur-intech commented Sep 17, 2018

artur-intech commented Sep 18, 2018

artur-intech commented Aug 12, 2018 •

edited

Loading

maciej-szlosarczyk Aug 14, 2018 •

edited

Loading

artur-intech Aug 16, 2018 •

edited

Loading

maciej-szlosarczyk Aug 14, 2018 •

edited

Loading

artur-intech commented Aug 16, 2018 •

edited

Loading