Merge pull request #1379 from geeksforsocialchange/main

Production deployment 6th July 2022

Gemfile.lock

@@ -393,7 +393,7 @@ GEM
       activesupport (>= 4.2)
       choice (~> 0.2.0)
       ruby-graphviz (~> 1.2)
-    rails-html-sanitizer (1.4.2)
+    rails-html-sanitizer (1.4.3)
       loofah (~> 2.3)
     rails_autolink (1.1.6)
       rails (> 3.1)

app/graphql/types/query_type.rb

@@ -60,7 +60,7 @@ def article_connection(**args)
     end
 
     def articles_by_tag(tag_id:)
-      Article.with_tag(tag_id).global_newsfeed.order(:title)
+      Article.with_tags(tag_id).global_newsfeed.order(:title)
     end
 
     def articles_by_partner_tag(tag_id:)
@@ -102,7 +102,7 @@ def ping
     end
   end
 
-  class QueryType < Types::BaseObject 
+  class QueryType < Types::BaseObject
 
     description "The base query schema for all of PlaceCal's GraphQL queries"
 

app/helpers/users_helper.rb

@@ -15,6 +15,9 @@ def options_for_roles
   end
 
   def user_has_no_rights?(user)
+    return false if user.root?
+    return false if user.editor?
+
     return false if user.tag_admin?
     return false if user.neighbourhood_admin?
     return false if user.partner_admin?

app/models/article.rb

@@ -27,20 +27,74 @@ class Article < ApplicationRecord
 
   scope :global_newsfeed, -> { published.order(published_at: :desc) }
 
-  scope :with_tag, ->(tag_id) { joins(:article_tags).where(article_tags: { tag: tag_id }) }
-
   scope :with_partner_tag, lambda { |tag_id|
     joins('left outer join article_partners on articles.id=article_partners.article_id')
     .joins('left outer join partner_tags on article_partners.partner_id = partner_tags.partner_id')
     .where('partner_tags.tag_id = ?', tag_id)
   }
 
+  scope :with_tags, lambda { |tag_ids|
+    joins(:article_tags).where(article_tags: { tag: tag_ids })
+  }
+
   scope :for_site, lambda { |site|
-    site_neighbourhood_ids = site.owned_neighbourhoods.pluck(:id)
 
-    joins(partners: [:address])
-      .where(address: { neighbourhood_id: site_neighbourhood_ids } )
-      .distinct
+    # this is a bit complicated but necessary
+    # the main problem to overcome is that we want articles by tag OR location
+    # (emphasis on OR).
+    #
+    # in simple AR land we would just chain scope methods like
+    #   `Article.by_tag(tags).by_location(neighbourhoods)` and be done with it
+    # each scope would set up its `joins` to pull in tables and its `wheres`
+    # to filter based on those joins. simple!
+    # but unfortunately these scopes get combined with 'AND' clauses in
+    # the `where` part.
+    #
+    # so what this does is build up the query for tags and locations like
+    # we were chaining scope methods without the conditions. we store
+    # the conditions as (clause, params) in an array as we go. when we reach
+    # the end we then extend the scope with a `where` clause that combines
+    # everything with `OR' as is needed and then return that to the
+    # calling code as a regular scope for further chaining.
+    #
+    # this also also allows us to skip an entire chunk of query if the
+    # site has no tags or locations.
+
+    site_neighbourhood_ids = site.owned_neighbourhoods.pluck(:id)
+    site_tag_ids = site.tags.pluck(:id)
+
+    # if site has no tags or neighbourhoods then just return nothing to caller
+    return none if site_neighbourhood_ids.empty? && site_tag_ids.empty?
+
+    scope = all
+
+    where_fragments = []
+    where_params = []
+
+    # articles by neighbourhood
+    if site_neighbourhood_ids.any?
+      # TODO: service areas?
+      scope = scope
+        .joins('LEFT OUTER JOIN article_partners ON articles.id=article_partners.article_id')
+        .joins('LEFT OUTER JOIN partners ON article_partners.partner_id = partners.id')
+        .joins('LEFT OUTER JOIN addresses ON partners.address_id = addresses.id')
+      where_fragments << 'addresses.neighbourhood_id IN (?)'
+      where_params << site_neighbourhood_ids
+    end
+
+    # articles by tag
+    if site_tag_ids.any?
+      scope = scope
+        .joins(' LEFT OUTER JOIN article_tags ON articles.id=article_tags.article_id')
+      where_fragments << 'article_tags.tag_id IN (?)'
+      where_params << site_tag_ids
+    end
+
+    # combine conditions with params to extend the scope
+    scope = scope
+      .where("(#{where_fragments.join(' OR ')})", *where_params)
+
+    scope.distinct('articles.id')
   }
 
   def update_published_at

app/models/partner.rb

@@ -223,6 +223,9 @@ def human_readable_opening_times
          <span class='opening_times--time'>#{o} &ndash; #{c}</span>
       ).html_safe
     end
+
+  rescue JSON::ParserError
+    []
   end
 
   def valid_public_phone?

app/views/admin/pages/home.html.erb

@@ -1,5 +1,10 @@
 <% content_for :title do %>Dashboard<% end %>
 
+<% if user_has_no_rights?(current_user) -%>
+  <h1 class="mt-5 mb-3">Missing Permissions</h1>
+  <p class='has-no-admin-rights-warning'>You have no admin rights. This probably means someone didn't set up your account correctly - please ask whoever invited you!</p>
+<% end -%>
+
 <% ['Partner', 'Calendar', 'User'].each do |model_name| %>
   <%- if policy(model_name.constantize).create? %>
     <%= link_to "Add New #{model_name}",
@@ -23,9 +28,9 @@
   </div>
 <% end %>
 
-<h1 class="mt-5 mb-3">Recently updated partners</h1>
-
 <% if @partners.any? %>
+
+  <h1 class="mt-5 mb-3">Recently updated partners</h1>
   <div class="card-grid">
     <% @partners.each do |partner| %>
       <%= render_component 'dashboard_card',

test/integration/admin/home_integration_test.rb

@@ -7,17 +7,30 @@ class AdminHomeIntegrationTest < ActionDispatch::IntegrationTest
 
   setup do
     @admin = create(:root)
+    @citizen = create(:citizen)
   end
 
   test "Admin home page can't be accessed without a login" do
     @default_site = create_default_site
     get "http://admin.lvh.me"
     assert_redirected_to "http://admin.lvh.me/users/sign_in"
+  end
+
+  test "Admin can access page when logged in" do
     sign_in @admin
     get "http://admin.lvh.me"
     assert_response :success
 
     assert_select 'title', text: "Dashboard | PlaceCal Admin"
-    assert_select 'h1', text: "Recently updated partners"
   end
+
+  test "Blank citizen gets a 'no content' warning" do
+    sign_in @citizen
+    get "http://admin.lvh.me"
+    assert_response :success
+
+    assert_select 'title', text: "Dashboard | PlaceCal Admin"
+    assert_select 'h1', text: "Missing Permissions"
+  end
+
 end

test/models/article_test.rb

@@ -55,7 +55,7 @@ class ArticleTest < ActiveSupport::TestCase
       article.tags << tag
     end
 
-    found = Article.with_tag(tag.id)
+    found = Article.with_tags(tag.id)
     assert_equal 2, found.count, 'Expected to only find articles with given tag'
   end
 
@@ -88,7 +88,7 @@ class ArticleTest < ActiveSupport::TestCase
     assert_equal 5, found.length, 'Expected to only find articles from tagged partners'
   end
 
-  test '::for_site returns articles for site' do
+  test '::for_site returns articles for site (via neighbourhood)' do
     neighbourhood_1 = neighbourhoods(:one)
     neighbourhood_2 = neighbourhoods(:two)
 
@@ -121,10 +121,66 @@ class ArticleTest < ActiveSupport::TestCase
       )
     end
 
-    found = Article.for_site(site)
+    found = Article.for_site(site).select(:id)
     assert_equal 5, found.count
+  end
+
+  test '::for_site returns articles with site tags applied' do
+    tag = create(:tag)
+    author = create(:root)
 
+    site = create(:site)
+    site.tags << tag
+    site.validate!
+
+    3.times do |n|
+      article = Article.create!(
+        title: "#{n} Article with tag",
+        is_draft: nil,
+        body: 'lorem ipsum dorem ditsum',
+        author: author
+      )
+      article.tags << tag
+      article.validate!
+    end
+
+    found = Article.for_site(site)
+    assert_equal 3, found.count
+  end
+
+  test '::for_site finds articles by both neighbourhood and tag' do
+    author = create(:root)
+    site = create(:site)
+
+    neighbourhood = neighbourhoods(:one)
+    site.neighbourhoods << neighbourhood
+
+    partner = create(:partner, address: create(:address, neighbourhood: neighbourhood))
+
+    3.times do |n|
+      partner.articles.create!(
+        title: "#{n} Article from Partner by neighbourhood",
+        is_draft: nil,
+        body: 'lorem ipsum dorem ditsum',
+        author: author
+      )
+    end
+
+    tag = create(:tag)
+    site.tags << tag
+
+    5.times do |n|
+      article = Article.create!(
+        title: "#{n} Article with tag",
+        is_draft: nil,
+        body: 'lorem ipsum dorem ditsum',
+        author: author
+      )
+      article.tags << tag
+    end
 
+    found = Article.for_site(site).select(:id)
+    assert_equal 8, found.count
   end
 
   # Unsure as to why this doesn't work. update_published_at triggers correctly

test/models/partner_test.rb

@@ -76,7 +76,7 @@ class PartnerTest < ActiveSupport::TestCase
 
   test 'validate description without summary' do
     @new_partner.update(
-      name: 'Test Partner', 
+      name: 'Test Partner',
       description: 'This is a test partner used for testing :)',
       summary: ''
     )
@@ -132,4 +132,11 @@ class PartnerTest < ActiveSupport::TestCase
     @new_partner.update(facebook_link: 'GroupName')
     refute @new_partner.errors.key?(:facebook_link), 'Valid page name not saved'
   end
+
+  test 'deals with badly formatted opening times' do
+    partner = build(:partner)
+    partner.opening_times = '{{ $data.openingHoursSpecifications }}'
+
+    assert_equal [], partner.human_readable_opening_times
+  end
 end