Use participatory spaces slugs in URLs #1842

mrcasals · 2017-09-14T13:33:33Z

🎩 What? Why?

Use process and assemblies slugs in URLs.

📌 Related Issues

Fixes Use slugs in participatory space's URLs #1820.

📋 Subtasks

Use process slugs in URLs
Use assembly slugs in URLs
Redirect old URLs with IDs to the new ones using the slugs
Fix specs
Validate slug format in model?

mrcasals · 2017-09-14T13:48:38Z

Currently, this solution doesn't work as I'd expect:

(note the menu highlighting)

I'll try to make a redirect when the param is an ID.

codecov · 2017-09-14T14:05:12Z

Codecov Report

Merging #1842 into master will decrease coverage by 0.02%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##           master    #1842      +/-   ##
==========================================
- Coverage    98.5%   98.47%   -0.03%     
==========================================
  Files        1127     1127              
  Lines       25284    25316      +32     
==========================================
+ Hits        24906    24931      +25     
- Misses        378      385       +7

oriolgual · 2017-09-14T14:06:23Z

...y_processes/app/constraints/decidim/participatory_processes/current_participatory_process.rb

@@ -30,7 +30,12 @@ def current_participatory_process(env, params)
      end

      def detect_current_participatory_process(params)
-        OrganizationParticipatoryProcesses.new(@organization).query.find_by_id(params["participatory_process_id"])
+        organization_processes.where(id: params["participatory_process_id"]).first ||
+          organization_processes.where(slug: params["participatory_process_id"]).first


Can't we use ActiveRecord's OR?

oriolgual · 2017-09-14T14:07:37Z

...sses/app/controllers/concerns/decidim/participatory_processes/needs_participatory_process.rb

@@ -42,7 +42,12 @@ def ability_context

        def detect_participatory_process
          request.env["current_participatory_process"] ||
-            OrganizationParticipatoryProcesses.new(current_organization).query.find(params[:participatory_process_id] || params[:id])
+            organization_processes.where(id: params[:participatory_process_id] || params[:id]).first ||


deivid-rodriguez · 2017-09-14T14:50:50Z

@mrcasals No big deal but IMO redirecting is better than serving the same content for both URLs with a 200 response. Search engines don't like it when different URLs serve the same content.

deivid-rodriguez · 2017-09-14T21:54:37Z

decidim-participatory_processes/lib/decidim/participatory_processes/engine.rb

+        get "/processes/:process_id/f/:feature_id", to: redirect { |params, _request|
+          process = Decidim::ParticipatoryProcess.where(id: params[:process_id]).first
+          process ? "/processes/#{process.slug}/f/#{params[:feature_id]}" : "/404"
+        }, constraints: { process_id: /[0-9]+/ }


@mrcasals Maybe you can use find(params[:process_id]) here. If the record is not found, an ActiveRecord::RecordNotFound error will be raised, which will result in a 404. I think that's better than manually redirecting to the "/404" page, since in that case the request will have a 200 status instead of a 404.

Also, to be technically correct, I guess this should be a fallback and the preferred routes should be matched first? I'm think of the super-unlikely case of numerical slugs.

Rails routes run the first definition that matches, so I need these redirections on top since they have the constraint.

Right, I see. The other alternative would be to manually redirect from the controller instead. But numerical slugs is a small edge case anyways.

mrcasals · 2017-09-15T07:02:34Z

@deivid-rodriguez good point. I added redirections in 09eb417 right before your comment, today I'll fix the /404 thing you point out 😄

beagleknight · 2017-09-18T10:33:41Z

decidim-assemblies/app/forms/decidim/assemblies/admin/assembly_form.rb

@@ -50,10 +50,14 @@ def scope
          @scope ||= current_organization.scopes.where(id: scope_id).first
        end

+        def to_param
+          id


This attribute is not defined, isn't it? I think you meant context[:assembly_id], right?

This code was being used, actually, so 🔥

beagleknight · 2017-09-18T10:37:46Z

decidim-core/lib/decidim/participable.rb

-        { host: organization.host, foreign_key.to_sym => id }
+        {
+          host: organization.host,
+          "#{underscored_name}_slug".to_sym => slug


Such hack lol 😄

beagleknight · 2017-09-18T10:38:33Z

...tory_processes/app/forms/decidim/participatory_processes/admin/participatory_process_form.rb

@@ -59,10 +59,14 @@ def participatory_process_group
          Decidim::ParticipatoryProcessGroup.where(id: participatory_process_group_id).first
        end

+        def to_param
+          id


beagleknight · 2017-09-18T10:40:03Z

decidim-assemblies/lib/decidim/assemblies/admin_engine.rb

@@ -14,14 +14,14 @@ class AdminEngine < ::Rails::Engine
      paths["db/migrate"] = nil

      routes do
-        resources :assemblies do
+        resources :assemblies, param: :slug, except: :show do


The param name slug why sometimes is called like this and sometimes like assembly_slug? Is it intended?

I simply renamed the old :id/:assembly_id to use the slug. I guess :assembly_id was used to differentiate assembly IDs from feature IDs. I don't want to change this because we might move features to use slugs in the URLs, so this is good.

beagleknight · 2017-09-18T10:40:17Z

decidim-participatory_processes/lib/decidim/participatory_processes/admin_engine.rb

@@ -15,7 +15,7 @@ class AdminEngine < ::Rails::Engine

      routes do
        resources :participatory_process_groups
-        resources :participatory_processes, except: :show do
+        resources :participatory_processes, param: :slug, except: :show do


josepjaume

Nice job! 🎉

Can you add an entry on the CHANGELOG indicating we use slugs on URLs now, and also a note about redirects?

deivid-rodriguez · 2017-09-18T10:54:36Z

decidim-assemblies/lib/decidim/assemblies/engine.rb

-        resources :assemblies, only: [:index, :show], path: "assemblies" do
+        get "assemblies/:assembly_id", to: redirect { |params, _request|
+          assembly = Decidim::Assembly.find(params[:assembly_id])
+          assembly ? "/assemblies/#{assembly.slug}" : "/404"


@mrcasals Isn't this line dead code now? find either rises or returns a record. I don't think we need the manual "/404" redirection anymore?

EDIT: Not the whole line, just the "/404" branch.

Rechecking this, the find method makes the constraint to fail, but I'm getting an authorization error instead of a nice 404 error:

I think I will go back to what we used to have, and then investigate again.

Definitely unexpected... 🤔

I think I will go back to what we used to have, and then investigate again.

Agree on the approach 👍

Wait, I misschecked it. This is why I'm getting with the current code:

The previous error appears after accessing a with a wrong slug: http://localhost:3000/assemblies/does-not-exist raises an authorization error instead of a 404.

I'll look into it, since it seems to be caused by this PR.

OK, last commit should fix it.

👍 If you want to test this stuff, I added some support for it a while ago. It's the a 404 page shared example.

oriolgual

Do we have format validations on the slug field to make sure we only accept valid chars (a-Z, 0-9 and - I guess)

oriolgual · 2017-09-18T12:39:05Z

CHANGELOG.md

@@ -6,6 +6,10 @@

 - **decidim-proposals**: Hide proposal vote button when proposal is answered and rejected. [\#1861](https://github.com/decidim/decidim/pull/1861)

+**Changed**
+
+- **decidim**: URLs now use the participatory space slug instead of the URL, both in the public pages and in the admin. Old routes using the space IDs now redirect to the ones using the slug. [\#1842](https://github.com/decidim/decidim/pull/1842)


Instead of the ID

We're not technically redirecting, only serving the same content (except for the participatory process and assembly page where we are indeed redirecting). There's a difference in terms of SEO, so I'd say "Old routes using the space IDs still serve the same content as the ones using the slug".

Mh I think it's a proper redirection, sending a 301 HHTP status code:

http://guides.rubyonrails.org/routing.html#redirection

I just tried it and it working perfectly! I was probably just misreading the patch, sorry! 😊

deivid-rodriguez · 2017-09-18T14:34:04Z

Did you notice this warning when seeding?

/home/deivid/Code/decidim/decidim-core/lib/decidim/participable.rb:12: warning: already initialized constant Decidim::Participable::SLUG_FORMAT

Also, can you generate a development app right now? I just had a dependency problem but I'm not sure if it's my environment...

deivid-rodriguez · 2017-09-18T14:37:59Z

decidim-assemblies/app/constraints/decidim/assemblies/current_assembly.rb

-        OrganizationAssemblies.new(@organization).query.find_by_id(params["assembly_id"])
+        organization_assemblies.where(slug: params["assembly_slug"]).or(
+          organization_assemblies.where(id: params["assembly_id"])
+        ).first!


I think this is what confused me... Do we need the assembly_id part? I tried and the redirection seems to work fine without it.

I'll check it, sure!

mrcasals · 2017-09-18T14:43:22Z

@deivid-rodriguez CircleCI has some problems:

Bundler::GitError: The git source https://github.com/rails/rails.git is not yet checked out. Please run `bundle install` before trying to start your application

I haven't generated the development app, let me check this tomorrow.

deivid-rodriguez · 2017-09-18T14:48:26Z

I'm having a look at it and it seems like a problem most likely caused by #1845...

mrcasals self-assigned this Sep 14, 2017

ghost added the in-progress label Sep 14, 2017

oriolgual reviewed Sep 14, 2017

View reviewed changes

deivid-rodriguez reviewed Sep 14, 2017

View reviewed changes

mrcasals force-pushed the use-spaces-slug-in-urls branch 3 times, most recently from f3ee3a7 to b6c6dcb Compare September 18, 2017 08:32

mrcasals changed the title ~~[WIP] Use process slugs in URLs~~ Use participatory spaces slugs in URLs Sep 18, 2017

mrcasals force-pushed the use-spaces-slug-in-urls branch from d89887b to 5de65a3 Compare September 18, 2017 10:13

mrcasals added in-review and removed in-progress labels Sep 18, 2017

beagleknight suggested changes Sep 18, 2017

View reviewed changes

beagleknight previously approved these changes Sep 18, 2017

View reviewed changes

mrcasals dismissed beagleknight’s stale review via cb0e987 September 18, 2017 10:49

mrcasals force-pushed the use-spaces-slug-in-urls branch from 52d03f9 to cb0e987 Compare September 18, 2017 10:49

josepjaume suggested changes Sep 18, 2017

View reviewed changes

deivid-rodriguez reviewed Sep 18, 2017

View reviewed changes

mrcasals force-pushed the use-spaces-slug-in-urls branch from e4277de to e595b06 Compare September 18, 2017 12:29

oriolgual suggested changes Sep 18, 2017

View reviewed changes

mrcasals force-pushed the use-spaces-slug-in-urls branch from e595b06 to a1461d2 Compare September 18, 2017 13:11

deivid-rodriguez reviewed Sep 18, 2017

View reviewed changes

mrcasals mentioned this pull request Sep 19, 2017

Fix dev apps dependencies #1866

Merged

mrcasals added 15 commits September 19, 2017 09:04

Use process slugs in URLs

60247e1

Make process routes with IDs redirect to slugs

7723d00

Fix process specs

1eea782

Improve code

a214eac

Do not use slugs in admin side

4fbf57e

Fix admin section

5787916

Fix some specs

fb56e70

Keep using slugs in admin

2d8560e

Fix more specs

96f68a8

Use slugs for assemblies URLs

12fb237

Simplify code

66733bc

Remove unused code

696f8c0

Force participatory space presence

04b1f75

Add CHANGELOG entry

15a3951

Validate slug format

6fd8567

mrcasals force-pushed the use-spaces-slug-in-urls branch from a1461d2 to 6fd8567 Compare September 19, 2017 07:04

Fix specs

1b85c62

mrcasals force-pushed the use-spaces-slug-in-urls branch from acaaa02 to ef7313a Compare September 19, 2017 07:58

Validate slug format from the model

ea8a29f

mrcasals force-pushed the use-spaces-slug-in-urls branch from ef7313a to ea8a29f Compare September 19, 2017 08:06

oriolgual approved these changes Sep 19, 2017

View reviewed changes

josepjaume approved these changes Sep 19, 2017

View reviewed changes

josepjaume merged commit 6ca97e1 into master Sep 19, 2017

josepjaume deleted the use-spaces-slug-in-urls branch September 19, 2017 12:33

ghost removed the in-review label Sep 19, 2017

beagleknight mentioned this pull request Sep 19, 2017

Move change to unreleased stage #1876

Merged

deivid-rodriguez mentioned this pull request Sep 20, 2017

Follow up to slug changes #1883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use participatory spaces slugs in URLs #1842

Use participatory spaces slugs in URLs #1842

mrcasals commented Sep 14, 2017 •

edited

Loading

mrcasals commented Sep 14, 2017

codecov bot commented Sep 14, 2017 •

edited

Loading

oriolgual Sep 14, 2017

oriolgual Sep 14, 2017

deivid-rodriguez commented Sep 14, 2017

deivid-rodriguez Sep 14, 2017

mrcasals Sep 15, 2017

deivid-rodriguez Sep 15, 2017

mrcasals commented Sep 15, 2017

beagleknight Sep 18, 2017

mrcasals Sep 18, 2017

beagleknight Sep 18, 2017

beagleknight Sep 18, 2017

beagleknight Sep 18, 2017

mrcasals Sep 18, 2017

beagleknight Sep 18, 2017

josepjaume left a comment

deivid-rodriguez Sep 18, 2017 •

edited

Loading

deivid-rodriguez Sep 18, 2017

mrcasals Sep 18, 2017

deivid-rodriguez Sep 18, 2017

mrcasals Sep 18, 2017

mrcasals Sep 18, 2017

deivid-rodriguez Sep 18, 2017

oriolgual left a comment

oriolgual Sep 18, 2017

deivid-rodriguez Sep 18, 2017

mrcasals Sep 18, 2017

deivid-rodriguez Sep 18, 2017

deivid-rodriguez commented Sep 18, 2017

deivid-rodriguez Sep 18, 2017

mrcasals Sep 18, 2017

mrcasals commented Sep 18, 2017

deivid-rodriguez commented Sep 18, 2017

Use participatory spaces slugs in URLs #1842

Use participatory spaces slugs in URLs #1842

Conversation

mrcasals commented Sep 14, 2017 • edited Loading

🎩 What? Why?

📌 Related Issues

📋 Subtasks

mrcasals commented Sep 14, 2017

codecov bot commented Sep 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcasals commented Sep 15, 2017

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

josepjaume left a comment

Choose a reason for hiding this comment

deivid-rodriguez Sep 18, 2017 • 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

oriolgual left a 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

deivid-rodriguez commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcasals commented Sep 18, 2017

deivid-rodriguez commented Sep 18, 2017

mrcasals commented Sep 14, 2017 •

edited

Loading

codecov bot commented Sep 14, 2017 •

edited

Loading

deivid-rodriguez Sep 18, 2017 •

edited

Loading