GDPR preparations for RESTful WHOIS #41

maciej-szlosarczyk · 2018-05-07T13:43:26Z

Fixes #37, #38 and #40.

Also:

removes dependency on HAML
adds complete test suite, both JSON and HTML
Fixes gemset name collision with registry project

* Add missing integration tests * Use Jbuilder for generating JSON * Refactor WhoisRecord model

* Remove gemset collision with registry * Remove Recaptcha api version, as only version 2 is supported

* Add tests that handle 404 requests * Add tests that handle POST HTTP verb

artur-intech · 2018-05-18T00:55:04Z

.ruby-gemset

@@ -1 +1 @@
-registry
+rest-whois


How do we use this file?

According to @teadur it has no bearing on production since servers are physically and logically separated.

However, locally it produces an undesired behaviour inside rvm/rbenv: both registry and and rest-whois used the same gemset, which for example contained 2 different versions of rails (4.2.10 and 4.2.7). It also allowed gems to "sneak" from one environment to another:

artur-intech · 2018-05-18T00:55:59Z

app/models/whois_record.rb

-  def full_body
-    @disclosed = (json['disclosed'] || []).dup
-    return body.to_s if @disclosed.empty?
+  BLOCKED = 'Blocked'.freeze


Perhaps it's worth using private_constant to hide them?

artur-intech · 2018-05-18T01:05:43Z

app/views/layouts/application.html.erb

+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
+<html lang="en">
+    <head>
+        <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"/>


This seems unnecessary

artur-intech · 2018-05-18T01:06:31Z

app/views/layouts/application.html.erb

+        <meta content="IE=edge" http-equiv="X-UA-Compatible"/>
+        <meta content="width=device-width, initial-scale=1" name="viewport"/>
+        <meta content="Full stack top-level domain (TLD) management." name="description"/>
+        <meta content="Estonian Internet Foundation" name="author"/>


How about removing the entire section? I don't see any point of it.

<meta content="IE=edge" http-equiv="X-UA-Compatible"/> <meta content="width=device-width, initial-scale=1" name="viewport"/> <meta content="Full stack top-level domain (TLD) management." name="description"/> <meta content="Estonian Internet Foundation" name="author"/>

artur-intech · 2018-05-18T01:10:46Z

app/views/shared/_flash.html.erb

@@ -0,0 +1,8 @@
+<% display = (flash[:notice] || flash[:alert] || flash[:warning]) ? 'block' : 'none' %>


Any chance to make this view more readable? Currently I find it hard to read.

artur-intech · 2018-05-18T03:13:01Z

app/views/whois_records/show.html.erb

+</pre>
+<% if @verified.blank? %>
+    <div class="full_info_form" style="padding: 20px 0 0 24px">
+        <%= form_for @whois_record, url: "/v1/#{@whois_record.name}", method: :get do |f| %>


Don't see f variable used. Wonder if we really need form_for here.

For all 3 comments above3: You'd have to ask Herbalizer, I converted it, since it worked, I didn't see any point in dwelling into the details of the rendered ERB.

artur-intech · 2018-05-18T03:31:42Z

app/views/whois_records/show.html.erb

+  <%= render partial: "whois", locals: { json: @whois_record.json } %>
+</pre>
+<% if @verified.blank? %>
+    <div class="full_info_form" style="padding: 20px 0 0 24px">


The convention is to use dashes instead of underscores for CSS entities.

artur-intech · 2018-05-19T05:26:04Z

test/models/whois_record_test.rb

+
+    @private_domain = whois_records(:privately_owned)
+    @legal_domain = whois_records(:legally_owned)
+    @discarded_domain = whois_records(:discarded)


Why WhoisRecord is assigned to a discarded_domain variable instead of whois_record? One can assume that he or she deals with a domain, whereas it is actually WHOIS-record.

Because whois_record_for_discarded_domain feels too long.

artur-intech · 2018-05-19T05:33:21Z

test/models/whois_record_test.rb

+
+  def test_partial_name_for_private_person
+    assert_equal('private_person', @private_domain.partial_name)
+    assert_equal('private_person', @private_domain.partial_name(true))


How about using keyword argument here? I need to dig in into that method's definition to find out what does that true mean.

artur-intech · 2018-05-19T07:10:10Z

app/views/whois_records/_legal_person_for_authorized.html.erb

@@ -0,0 +1,57 @@
+Domain:


There is a duplication between legal_person_for_authorized.json.jbuilder and legal_person.json.jbuilder. I see a clear pattern of undisclosed data ("Not Disclosed - Visit www.internet.ee for webbased WHOIS").

Perhaps we could unify those templates?

I decided against it since you end up with an if block, something I find rather unreadable in templates. I'd rather live with the repetition.

if @verified json.array!(whois_record.json['tech_contacts']) do |_contact| json.name 'Not Disclosed' json.email 'Not Disclosed' json.changed 'Not Disclosed' end else json.array!(whois_record.json['admin_contacts']) do |_contact| json.name 'Not Disclosed - Visit www.internet.ee for webbased WHOIS' json.email 'Not Disclosed - Visit www.internet.ee for webbased WHOIS' json.changed 'Not Disclosed - Visit www.internet.ee for webbased WHOIS' end end

I prefer having multiple templates to one that covers multiple, hence the design where WhoisRecord instance tells the controller which template to render, not the controller/template asking the WhoisRecord about its properties.

#37

artur-intech · 2018-05-21T07:09:11Z

@vohmar We have discussed above mentioned issues with @maciej-szlosarczyk so you're free to merge it once tested. And the sooner the better, since there are other upcoming changes.

maciej-szlosarczyk added 11 commits May 3, 2018 12:31

Prepare test environment

7ac8f71

Add JSON response tests

1c2b382

Write tests required before writing updates for GDPR

3b13ab4

Replace HAML with ERB, remove HAML gem

f17a740

Refactor views

40a2032

* Add missing integration tests * Use Jbuilder for generating JSON * Refactor WhoisRecord model

Clean up configuration variables

d3cffdc

* Remove gemset collision with registry * Remove Recaptcha api version, as only version 2 is supported

Refactor the views and the controller

3bc0e5e

Add model tests

bd402ee

Restore JSON handling of 404

a97a657

Broaden the test coverage

ce8eb50

* Add tests that handle 404 requests * Add tests that handle POST HTTP verb

Add handling of Blocked, Reserved and deleteCandidate

bf5ead7

maciej-szlosarczyk force-pushed the rest-whois-37 branch 2 times, most recently from 5b36ba1 to 8e6a4bb Compare May 8, 2018 12:29

Remove unecessary whitespace from HTML template

bcba344

maciej-szlosarczyk force-pushed the rest-whois-37 branch from 8e6a4bb to bcba344 Compare May 14, 2018 11:09

artur-intech reviewed May 19, 2018

View reviewed changes

Artur Beljajev added 2 commits May 21, 2018 07:06

Fix test names

65c50fd

#37

Hard-code setting in tests

0091c57

#37

artur-intech force-pushed the rest-whois-37 branch from c79bf2d to 0091c57 Compare May 21, 2018 05:10

Merge branch 'master' into rest-whois-37

7abbc9f

maciej-szlosarczyk mentioned this pull request May 22, 2018

Add ability to send emails to private domain owners #50

Merged

vohmar merged commit 2b79fdb into master May 23, 2018

maciej-szlosarczyk mentioned this pull request May 24, 2018

Remove any reference of UNIX body in REST WHOIS #40

Closed

vohmar deleted the rest-whois-37 branch May 25, 2018 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GDPR preparations for RESTful WHOIS #41

GDPR preparations for RESTful WHOIS #41

maciej-szlosarczyk commented May 7, 2018

artur-intech May 18, 2018

maciej-szlosarczyk May 19, 2018

artur-intech May 18, 2018

artur-intech May 18, 2018

artur-intech May 18, 2018

artur-intech May 18, 2018 •

edited

Loading

artur-intech May 18, 2018

maciej-szlosarczyk May 19, 2018

artur-intech May 18, 2018

artur-intech May 19, 2018

maciej-szlosarczyk May 19, 2018

artur-intech May 19, 2018

artur-intech May 19, 2018

maciej-szlosarczyk May 19, 2018 •

edited

Loading

artur-intech commented May 21, 2018 •

edited

Loading

		@@ -0,0 +1,8 @@
		<% display = (flash[:notice] \|\| flash[:alert] \|\| flash[:warning]) ? 'block' : 'none' %>

		@@ -1 +1 @@
		registry
		rest-whois

GDPR preparations for RESTful WHOIS #41

GDPR preparations for RESTful WHOIS #41

Conversation

maciej-szlosarczyk commented May 7, 2018

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

artur-intech May 18, 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

maciej-szlosarczyk May 19, 2018 • edited Loading

Choose a reason for hiding this comment

artur-intech commented May 21, 2018 • edited Loading

artur-intech May 18, 2018 •

edited

Loading

maciej-szlosarczyk May 19, 2018 •

edited

Loading

artur-intech commented May 21, 2018 •

edited

Loading