Skip to content

Allow an assembly to have children#2938

Merged
mrcasals merged 23 commits intomasterfrom
feature/relationship-between-assemblies
Apr 3, 2018
Merged

Allow an assembly to have children#2938
mrcasals merged 23 commits intomasterfrom
feature/relationship-between-assemblies

Conversation

@rbngzlv
Copy link
Copy Markdown
Contributor

@rbngzlv rbngzlv commented Mar 7, 2018

🎩 What? Why?

It must be possible for an assembly to have children

📌 Related Issues

📋 Subtasks

  • Add specs for public views
  • Add CHANGELOG entry
  • Add specs for ancestors_paths

@ghost ghost assigned rbngzlv Mar 7, 2018
@ghost ghost added the in-progress label Mar 7, 2018
scopes_enabled: form.scopes_enabled,
scope: form.scope,
area: form.area,
parent_id: form.parent_id,
Copy link
Copy Markdown
Contributor

@mrcasals mrcasals Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to modify the Decidim::Assemblies::AdminLog::AssemblyPresenter to reflect this attribute

Copy link
Copy Markdown
Contributor

@oriolgual oriolgual Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the object instead of the id? (parent: form.parent)

@carolromero carolromero mentioned this pull request Mar 7, 2018
6 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 7, 2018

Codecov Report

Merging #2938 into master will increase coverage by <.01%.
The diff coverage is 99.47%.

@@            Coverage Diff            @@
##           master   #2938      +/-   ##
=========================================
+ Coverage   98.59%   98.6%   +<.01%     
=========================================
  Files        1716    1718       +2     
  Lines       40967   41105     +138     
=========================================
+ Hits        40393   40530     +137     
- Misses        574     575       +1

@rbngzlv rbngzlv force-pushed the feature/relationship-between-assemblies branch from a480143 to 41c0ff9 Compare March 7, 2018 18:19
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Mar 8, 2018

@decidim/lot-core ready to review!

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 9, 2018

@rbngzlv some conflicts arose! 😄

slug
end

def ancestors
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we save these ancestors as an array of IDs in the DB, so we can speed up the relation? If we save an array, then we can do Assembly.where(id: ancestor_ids) and we can return them all at once instead of querying them one by one

@rbngzlv rbngzlv force-pushed the feature/relationship-between-assemblies branch from c99284f to bcd1bcb Compare March 12, 2018 07:51
<div class="card-divider">
<h2 class="card-title">
<%= t "decidim.admin.titles.assemblies" %><%= link_to t("actions.new", scope: "decidim.admin", name: t("models.assembly.name", scope: "decidim.admin")), ["new", "assembly"], class: "button tiny button--title" if can? :create, Decidim::Assembly %>
<%= link_to "#{translated_attribute(parent_assembly.title)} > ", edit_assembly_path(parent_assembly) if parent_assembly %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we only having 1 level of parent-child? If we're having multiple levels of parents, then this should reflect all the ancestors, I guess?

mrcasals
mrcasals previously approved these changes Mar 12, 2018
private

# rubocop:disable Rails/SkipsModelValidations
def set_parents_path
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbngzlv can you explain the paths concept? I don't understand it

Copy link
Copy Markdown
Contributor Author

@rbngzlv rbngzlv Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oriolgual @mrcasals asked me to have the parent_ids cached in a column, so we can render the breadcumbs faster without a 1+N query. I have implemented it using the ltree extension of PostgreSQL.

This allow us to update all descendants (child of child, ...) of an assembly in a single query when their parent changes and also to save the "path" for the actual record only querying their direct parent.

You can check this blog post for more information: http://patshaughnessy.net/2017/12/13/saving-a-tree-in-postgres-using-ltree

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be nice to add some docs explaining this then, since it's not quite common.


class EnableLtreeExtension < ActiveRecord::Migration[5.0]
def change
enable_extension "ltree"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we merge these migrations?

scopes_enabled: form.scopes_enabled,
scope: form.scope,
area: form.area,
parent_id: form.parent_id,
Copy link
Copy Markdown
Contributor

@oriolgual oriolgual Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the object instead of the id? (parent: form.parent)

scopes_enabled: form.scopes_enabled,
scope: form.scope,
area: form.area,
parent_id: form.parent_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as create, use the object instead of the id.

rbngzlv added 2 commits March 21, 2018 12:50
* master:
  [RFC] Use cells for meeting m cards (#3022)
  Do not force Postgresql user to be admin when enabling trigram extension (#3053)
  Make organization reference_prefix required (#3056)
  admin can duplicate/copy meetings (#3051)
  Fix question form errors not being displayed (#3046)
  Erb whitespace cutting (#3047)
  Show debates statistics on space show and homepage (#3016)
  Fix broken translated field after form errors (#3026)
  Move decidim executable to "exe" folder (#3028)
  Friendlier buttons (#3027)
  Feedback needed after Endorsing when user has no user_groups (#2998)
  Fix seeding error on generator specs (#3021)
  fix spelling error in threshold (#3019)
  Migration plus seeds (#2933)
@rbngzlv rbngzlv force-pushed the feature/relationship-between-assemblies branch from 39422c9 to 31f05ec Compare March 21, 2018 11:52
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Mar 21, 2018

@mrcasals @oriolgual I think that all requested changes are done!

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Mar 26, 2018

@decidim/lot-core can you review it, please?

@mrcasals mrcasals merged commit f0a41e6 into master Apr 3, 2018
@ghost ghost removed the status: WIP label Apr 3, 2018
@mrcasals mrcasals deleted the feature/relationship-between-assemblies branch April 3, 2018 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants