Contacts API Feature complete with full tests

app/controllers/api/v1/contacts_controller.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+module Api
+  module V1
+    class ContactsController < Api::V1::BaseController
+      before_action only: %i[index] do
+        require_access_token %w[read]
+      end
+
+      before_action only: %i[create destroy] do
+        require_access_token %w[read write]
+      end
+
+      rescue_from ActiveRecord::RecordNotFound do
+        render json: I18n.t("api.endpoint_errors.aspects.not_found"), status: :not_found
+      end
+
+      def index
+        contacts = aspects_membership_service.contacts_in_aspect(params[:aspect_id])
+        render json: contacts.map {|c| ContactPresenter.new(c, current_user).as_api_json_without_contact }
+      end
+
+      def create
+        aspect_id = params[:aspect_id]
+        person = Person.find_by(guid: params[:person_guid])
+        aspect_membership = aspects_membership_service.create(aspect_id, person.id) if person.present?
+        if aspect_membership
+          head :no_content
+        else
+          render json: I18n.t("api.endpoint_errors.contacts.cant_create"), status: :unprocessable_entity
+        end
+      rescue ActiveRecord::RecordNotUnique
+        render json: I18n.t("api.endpoint_errors.contacts.cant_create"), status: :unprocessable_entity
+      end
+
+      def destroy
+        aspect_id = params[:aspect_id]
+        person = Person.find_by(guid: params[:id])
+        result = aspects_membership_service.destroy_by_ids(aspect_id, person.id) if person.present?
+        if result && result[:success]
+          head :no_content
+        else
+          render json: I18n.t("api.endpoint_errors.contacts.cant_delete"), status: :unprocessable_entity
+        end
+      rescue ActiveRecord::RecordNotFound
+        render json: I18n.t("api.endpoint_errors.contacts.not_found"), status: :not_found
+      end
+
+      def aspects_membership_service
+        AspectsMembershipService.new(current_user)
+      end
+    end
+  end
+end

app/controllers/aspect_memberships_controller.rb

@@ -11,20 +11,9 @@ class AspectMembershipsController < ApplicationController
   respond_to :json
 
   def destroy
-    aspect = current_user.aspects.joins(:aspect_memberships).where(aspect_memberships: {id: params[:id]}).first
+    delete_results = AspectsMembershipService.new(current_user).destroy_by_membership_id(params[:id])
-    contact = current_user.contacts.joins(:aspect_memberships).where(aspect_memberships: {id: params[:id]}).first
+    success = delete_results[:success]
-
+    membership = delete_results[:membership]
-    raise ActiveRecord::RecordNotFound unless aspect.present? && contact.present?
-
-    raise Diaspora::NotMine unless current_user.mine?(aspect) &&
-                                   current_user.mine?(contact)
-
-    membership = contact.aspect_memberships.where(aspect_id: aspect.id).first
-
-    raise ActiveRecord::RecordNotFound unless membership.present?
-
-    # do it!
-    success = membership.destroy
 
     # set the flash message
     respond_to do |format|
@@ -39,17 +28,12 @@ class AspectMembershipsController < ApplicationController
   end
 
   def create
-    @person = Person.find(params[:person_id])
+    aspect_membership = AspectsMembershipService.new(current_user).create(params[:aspect_id], params[:person_id])
-    @aspect = current_user.aspects.where(id: params[:aspect_id]).first
 
-    @contact = current_user.share_with(@person, @aspect)
+    if aspect_membership
-
-    if @contact.present?
       respond_to do |format|
         format.json do
-          render json: AspectMembershipPresenter.new(
+          render json: AspectMembershipPresenter.new(aspect_membership).base_hash
-            AspectMembership.where(contact_id: @contact.id, aspect_id: @aspect.id).first)
-          .base_hash
         end
       end
     else
@@ -59,6 +43,12 @@ class AspectMembershipsController < ApplicationController
         end
       end
     end
+  rescue RuntimeError
+    respond_to do |format|
+      format.json do
+        render plain: I18n.t("aspects.add_to_aspect.failure"), status: :conflict
+      end
+    end
   end
 
   rescue_from ActiveRecord::StatementInvalid do

app/presenters/contact_presenter.rb

@@ -17,6 +17,9 @@ class ContactPresenter < BasePresenter
     full_hash.merge(person: person_without_contact)
   end
 
+  def as_api_json_without_contact
+    PersonPresenter.new(person, current_user).as_api_json
+  end
   private
 
   def person_without_contact

app/services/aspects_membership_service.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+class AspectsMembershipService
+  def initialize(user=nil)
+    @user = user
+  end
+
+  def create(aspect_id, person_id)
+    person = Person.find(person_id)
+    aspect = @user.aspects.where(id: aspect_id).first
+    raise ActiveRecord::RecordNotFound unless person.present? && aspect.present?
+
+    contact = @user.share_with(person, aspect)
+    raise I18n.t("aspects.add_to_aspect.failure") if contact.blank?
+
+    AspectMembership.where(contact_id: contact.id, aspect_id: aspect.id).first
+  end
+
+  def destroy_by_ids(aspect_id, contact_id)
+    aspect = @user.aspects.where(id: aspect_id).first
+    contact = @user.contacts.where(person_id: contact_id).first
+    destroy(aspect, contact)
+  end
+
+  def destroy_by_membership_id(membership_id)
+    aspect = @user.aspects.joins(:aspect_memberships).where(aspect_memberships: {id: membership_id}).first
+    contact = @user.contacts.joins(:aspect_memberships).where(aspect_memberships: {id: membership_id}).first
+    destroy(aspect, contact)
+  end
+
+  def contacts_in_aspect(aspect_id)
+    order = ["contact_id IS NOT NULL DESC", "profiles.first_name ASC", "profiles.last_name ASC",
+             "profiles.diaspora_handle ASC"]
+    @user.aspects.find(aspect_id) # to provide better error code if aspect isn't correct
+    contacts = @user.contacts.arel_table
+    aspect_memberships = AspectMembership.arel_table
+    @user.contacts.joins(
+      contacts.join(aspect_memberships).on(
+        aspect_memberships[:aspect_id].eq(aspect_id).and(
+          aspect_memberships[:contact_id].eq(contacts[:id])
+        )
+      ).join_sources
+    ).includes(person: :profile).order(order)
+  end
+
+  private
+
+  def destroy(aspect, contact)
+    raise ActiveRecord::RecordNotFound unless aspect.present? && contact.present?
+    raise Diaspora::NotMine unless @user.mine?(aspect) && @user.mine?(contact)
+
+    membership = contact.aspect_memberships.where(aspect_id: aspect.id).first
+    raise ActiveRecord::RecordNotFound if membership.blank?
+
+    success = membership.destroy
+
+    {success: success, membership: membership}
+  end
+end

config/locales/diaspora/en.yml

@@ -964,6 +964,10 @@ en:
         not_allowed: "User is not allowed to comment"
         no_delete: "User not allowed to delete the comment"
         duplicate_report: "This item already has been reported by this user"
+      contacts:
+        not_found: "Aspect or contact on aspect not found"
+        cant_create: "Failed to add user to aspect"
+        cant_delete: "Failed to remove user from aspect"          
       likes:
         user_not_allowed_to_like: "User is not allowed to like"
         like_exists: "Like already exists"

config/routes.rb

@@ -222,7 +222,9 @@ Rails.application.routes.draw do
   get "podmin", to: "home#podmin"
 
   api_version(module: "Api::V1", path: {value: "api/v1"}) do
-    resources :aspects, only: %i[show index create destroy update]
+    resources :aspects, only: %i[show index create destroy update] do
+      resources :contacts, only: %i[index create destroy]
+    end
     resources :posts, only: %i[show create destroy] do
       resources :comments, only: %i[create index destroy] do
         post "report" => "comments#report"

spec/integration/api/contacts_controller_spec.rb

@@ -0,0 +1,233 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe Api::V1::ContactsController do
+  let(:auth) { FactoryGirl.create(:auth_with_read_and_write) }
+  let(:auth_read_only) { FactoryGirl.create(:auth_with_read) }
+  let!(:access_token) { auth.create_access_token.to_s }
+  let!(:access_token_read_only) { auth_read_only.create_access_token.to_s }
+
+  before do
+    @aspect1 = auth.user.aspects.where(name: "generic").first
+    @aspect2 = auth.user.aspects.create(name: "another aspect")
+    @eve_aspect = eve.aspects.first
+  end
+
+  describe "#show" do
+    before do
+      aspects_membership_service.create(@aspect2.id, alice.person.id)
+    end
+
+    context "for valid aspect" do
+      it "lists members" do
+        get(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(200)
+        contacts = JSON.parse(response.body)
+        expect(contacts.length).to eq(1)
+        confirm_person_format(contacts[0], alice)
+
+        get(
+          api_v1_aspect_contacts_path(@aspect1.id),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(200)
+        contacts = JSON.parse(response.body)
+        expect(contacts.length).to eq(@aspect1.contacts.length)
+      end
+    end
+
+    context "for invalid aspect" do
+      it "fails for non-existant Aspect ID" do
+        get(
+          api_v1_aspect_contacts_path(-1),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.aspects.not_found"))
+      end
+
+      it "fails for other user's Aspect ID" do
+        get(
+          api_v1_aspect_contacts_path(@eve_aspect.id),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.aspects.not_found"))
+      end
+    end
+
+    context "improper credentials" do
+      it "fails when not logged in" do
+        get(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {access_token: "999_999_999"}
+        )
+        expect(response.status).to eq(401)
+      end
+    end
+  end
+
+  describe "#create" do
+    context "with valid person GUID and aspect" do
+      it "succeeds adding" do
+        post(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {person_guid: alice.guid, access_token: access_token}
+        )
+        expect(response.status).to eq(204)
+        expect(@aspect2.contacts.length).to eq(1)
+      end
+
+      it "fails if re-adding" do
+        aspects_membership_service.create(@aspect2.id, alice.person.id)
+        post(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {person_guid: alice.guid, access_token: access_token}
+        )
+        expect(response.status).to eq(422)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.contacts.cant_create"))
+      end
+    end
+
+    context "with invalid Aspect" do
+      it "fails for non-existant Aspect ID" do
+        post(
+          api_v1_aspect_contacts_path(-1),
+          params: {person_guid: alice.guid, access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.aspects.not_found"))
+      end
+
+      it "fails for other user's Aspect ID" do
+        post(
+          api_v1_aspect_contacts_path(@eve_aspect.id),
+          params: {person_guid: alice.guid, access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.aspects.not_found"))
+      end
+    end
+
+    context "with invalid person GUID" do
+      it "fails to add" do
+        post(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {person_guid: "999_999_999", access_token: access_token}
+        )
+        expect(response.status).to eq(422)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.contacts.cant_create"))
+      end
+    end
+
+    context "improper credentials" do
+      it "fails when not logged in" do
+        post(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {person_guid: alice.guid, access_token: "999_999_999"}
+        )
+        expect(response.status).to eq(401)
+      end
+
+      it "fails when only read only token" do
+        post(
+          api_v1_aspect_contacts_path(@aspect2.id),
+          params: {person_guid: alice.guid, access_token: access_token_read_only}
+        )
+        expect(response.status).to eq(403)
+      end
+    end
+  end
+
+  describe "#destroy" do
+    before do
+      aspects_membership_service.create(@aspect2.id, alice.person.id)
+    end
+
+    context "with valid person GUID and aspect" do
+      it "succeeds deleting" do
+        delete(
+          api_v1_aspect_contact_path(@aspect2.id, alice.guid),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(204)
+        expect(@aspect2.contacts.length).to eq(0)
+      end
+
+      it "fails if not in aspect" do
+        delete(
+          api_v1_aspect_contact_path(@aspect2.id, eve.guid),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.contacts.not_found"))
+      end
+    end
+
+    context "with invalid Aspect" do
+      it "fails for non-existant Aspect ID" do
+        delete(
+          api_v1_aspect_contact_path(-1, eve.guid),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.contacts.not_found"))
+      end
+
+      it "fails for other user's Aspect ID" do
+        delete(
+          api_v1_aspect_contact_path(@eve_aspect.id, eve.guid),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(404)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.contacts.not_found"))
+      end
+    end
+
+    context "with invalid person GUID" do
+      it "fails to delete " do
+        delete(
+          api_v1_aspect_contact_path(@aspect2.id, "999_999_999"),
+          params: {access_token: access_token}
+        )
+        expect(response.status).to eq(422)
+        expect(response.body).to eq(I18n.t("api.endpoint_errors.contacts.cant_delete"))
+      end
+    end
+
+    context "improper credentials" do
+      it "fails when not logged in" do
+        delete(
+          api_v1_aspect_contact_path(@aspect2.id, alice.guid),
+          params: {access_token: "999_999_999"}
+        )
+        expect(response.status).to eq(401)
+      end
+
+      it "fails when only read only token" do
+        delete(
+          api_v1_aspect_contact_path(@aspect2.id, alice.guid),
+          params: {access_token: access_token_read_only}
+        )
+        expect(response.status).to eq(403)
+      end
+    end
+  end
+
+  def aspects_membership_service(user=auth.user)
+    AspectsMembershipService.new(user)
+  end
+
+  # rubocop:disable Metrics/AbcSize
+  def confirm_person_format(post_person, user)
+    expect(post_person["guid"]).to eq(user.guid)
+    expect(post_person["diaspora_id"]).to eq(user.diaspora_handle)
+    expect(post_person["name"]).to eq(user.name)
+    expect(post_person["avatar"]).to eq(user.profile.image_url)
+  end
+  # rubocop:enable Metrics/AbcSize
+end

spec/integration/api/likes_controller_spec.rb

@@ -1,4 +1,4 @@
-# frozen_sTring_literal: true
+# frozen_string_literal: true
 
 require "spec_helper"
 

spec/integration/api/reshares_controller_spec.rb

@@ -145,7 +145,6 @@ describe Api::V1::ResharesController do
           params: {access_token: access_token}
         )
 
-        puts(response.body)
         expect(response.status).to eq(422)
         expect(response.body).to eq(I18n.t("reshares.create.error"))
       end

spec/services/aspects_membership_service_spec.rb

@@ -0,0 +1,137 @@
+# frozen_string_literal: true
+
+describe AspectsMembershipService do
+  before do
+    @alice_aspect1  = alice.aspects.first
+    @alice_aspect2  = alice.aspects.create(name: "another aspect")
+    @bob_aspect1 = bob.aspects.first
+  end
+
+  describe "#create" do
+    context "with valid IDs" do
+      it "succeeds" do
+        membership = aspects_membership_service.create(@alice_aspect2.id, bob.person.id)
+        expect(membership[:aspect_id]).to eq(@alice_aspect2.id)
+        expect(@alice_aspect2.contacts.find_by(person_id: bob.person.id)).not_to be_nil
+      end
+
+      it "fails if already in aspect" do
+        aspects_membership_service.create(@alice_aspect2.id, bob.person.id)
+        expect {
+          aspects_membership_service.create(@alice_aspect2.id, bob.person.id)
+        }.to raise_error ActiveRecord::RecordNotUnique
+      end
+    end
+
+    context "with invalid IDs" do
+      it "fails with invalid User ID" do
+        expect {
+          aspects_membership_service.create(@alice_aspect2.id, -1)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+
+      it "fails with invalid Aspect ID" do
+        expect {
+          aspects_membership_service.create(-1, bob.person.id)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+
+      it "fails with aspect ID that isn't user's" do
+        expect {
+          aspects_membership_service.create(@bob_aspect1.id, eve.person.id)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+    end
+  end
+
+  describe "#destroy" do
+    before do
+      @membership = aspects_membership_service.create(@alice_aspect2.id, bob.person.id)
+    end
+
+    context "with aspect/user valid IDs" do
+      it "succeeds if in aspect" do
+        aspects_membership_service.destroy_by_ids(@alice_aspect2.id, bob.person.id)
+        expect(@alice_aspect2.contacts.find_by(person_id: bob.person.id)).to be_nil
+      end
+
+      it "fails if not in aspect" do
+        expect {
+          aspects_membership_service.destroy_by_ids(@alice_aspect2.id, eve.person.id)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+    end
+
+    context "with a membership ID" do
+      it "succeeds if their membership" do
+        aspects_membership_service.destroy_by_membership_id(@membership.id)
+        expect(@alice_aspect2.contacts.find_by(person_id: bob.person.id)).to be_nil
+      end
+
+      it "fails if not their membership" do
+        expect {
+          aspects_membership_service(eve).destroy_by_membership_id(@membership.id)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+
+      it "fails if invalid membership ID" do
+        expect {
+          aspects_membership_service(eve).destroy_by_membership_id(-1)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+    end
+
+    context "with invalid IDs" do
+      it "fails with invalid User ID" do
+        expect {
+          aspects_membership_service.destroy_by_ids(@alice_aspect2.id, -1)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+
+      it "fails with invalid Aspect ID" do
+        expect {
+          aspects_membership_service.destroy_by_ids(-1, eve.person.id)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+
+      it "fails with aspect ID that isn't user's" do
+        expect {
+          aspects_membership_service(eve).destroy_by_ids(@alice_aspect2.id, bob.person.id)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+    end
+  end
+
+  describe "#list" do
+    before do
+      aspects_membership_service.create(@alice_aspect2.id, bob.person.id)
+      aspects_membership_service.create(@alice_aspect2.id, eve.person.id)
+      @alice_aspect3 = alice.aspects.create(name: "empty aspect")
+    end
+
+    context "with valid aspect ID" do
+      it "returns users in full aspect" do
+        contacts = aspects_membership_service.contacts_in_aspect(@alice_aspect2.id)
+        expect(contacts.length).to eq(2)
+        expect(contacts.map {|c| c.person.guid }.sort).to eq([bob.person.guid, eve.person.guid].sort)
+      end
+
+      it "returns empty array in empty aspect" do
+        contacts = aspects_membership_service.contacts_in_aspect(@alice_aspect3.id)
+        expect(contacts.length).to eq(0)
+      end
+    end
+
+    context "with invalid aspect ID" do
+      it "fails" do
+        expect {
+          aspects_membership_service.contacts_in_aspect(-1)
+        }.to raise_error ActiveRecord::RecordNotFound
+      end
+    end
+  end
+
+  def aspects_membership_service(user=alice)
+    AspectsMembershipService.new(user)
+  end
+end