Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add local targets to related list selector #4327

Merged
merged 4 commits into from
Jan 26, 2021
Merged

Add local targets to related list selector #4327

merged 4 commits into from
Jan 26, 2021

Conversation

taitus
Copy link
Member

@taitus taitus commented Jan 22, 2021 •
edited

References

Related PR's: #4322 #4315

Objectives

Allow add local targets to RelatedListSelectorComponent

Visual Changes

Captura de pantalla 2021-01-23 a las 14 03 23

@taitus taitus added the SDG United Nations Sustainable Development Goals label Jan 22, 2021
@taitus taitus self-assigned this Jan 22, 2021
@taitus taitus added this to Reviewing in Consul Democracy via automation Jan 22, 2021
@taitus taitus changed the base branch from master to sdg_local_filter January 22, 2021 12:49
@taitus taitus moved this from Reviewing to Doing in Consul Democracy Jan 22, 2021
Base automatically changed from sdg_local_filter to master January 22, 2021 15:48
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch 2 times, most recently from 27efd28 to 9237fdb Compare January 23, 2021 13:01
@taitus taitus marked this pull request as ready for review January 23, 2021 13:02
@taitus taitus changed the title [WIP] Add local targets to related list selector Add local targets to related list selector Jan 23, 2021
@taitus taitus moved this from Doing to Reviewing in Consul Democracy Jan 23, 2021
@javierm javierm self-assigned this Jan 25, 2021
javierm
javierm reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

Here's an initial review. Let me know what you think!

app/controllers/sdg_management/local_targets_controller.rb Outdated Show resolved Hide resolved
app/models/sdg/goal.rb Outdated Show resolved Hide resolved
spec/components/sdg/related_list_selector_component_spec.rb Outdated Show resolved Hide resolved
spec/components/sdg/related_list_selector_component_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/relatable_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/relatable_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/relatable_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/relatable_spec.rb Outdated Show resolved Hide resolved
app/models/concerns/sdg/relatable.rb Outdated Show resolved Hide resolved
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jan 25, 2021
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch from 9237fdb to bc3ce8d Compare January 25, 2021 17:07
@taitus taitus moved this from Doing to Reviewing in Consul Democracy Jan 25, 2021
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch from bc3ce8d to 833bc80 Compare January 25, 2021 18:54
javierm
javierm reviewed Jan 26, 2021
View reviewed changes
app/models/concerns/sdg/relatable.rb Show resolved Hide resolved
app/models/sdg/local_target.rb Show resolved Hide resolved
db/dev_seeds/sdg.rb Outdated Show resolved Hide resolved
spec/models/sdg/goal_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/goal_spec.rb Show resolved Hide resolved
spec/models/sdg/local_target_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/local_target_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/target_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/relatable_spec.rb Outdated Show resolved Hide resolved
spec/system/sdg_management/relations_spec.rb Outdated Show resolved Hide resolved
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jan 26, 2021
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch 2 times, most recently from 47ea94a to ecbf387 Compare January 26, 2021 17:08
@taitus taitus moved this from Doing to Reviewing in Consul Democracy Jan 26, 2021
@taitus taitus moved this from Reviewing to Doing in Consul Democracy Jan 26, 2021
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch from ecbf387 to 70e1e74 Compare January 26, 2021 17:49
@taitus taitus moved this from Doing to Reviewing in Consul Democracy Jan 26, 2021
javierm and others added 2 commits January 26, 2021 19:10
@javierm @taitus
Add method to easily access a local target by code
cb57a46 
Similar to what we do in goals and targets.
@taitus
Add relation between Goal and LocalTarget
0a3de68 
This is similar to what we do with investments, which belong to a heading
but also belong to a budget. In our case, the reason is we've been asked
to add local targets which belong to a goal but are not related to any
existing target.
Even though we're not implementing that case right now, we're adding the
relation so we don't have to add data migrations in the future.
javierm
javierm reviewed Jan 26, 2021
View reviewed changes
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

Looks great! I've left a couple of comments regarding typos.

spec/models/sdg/goal_spec.rb Outdated Show resolved Hide resolved
spec/models/sdg/relatable_spec.rb Outdated Show resolved Hide resolved
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch from 70e1e74 to cd5fbb9 Compare January 26, 2021 18:10
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jan 26, 2021
@taitus
Allow add local targets to RelatedListSelectorComponent
41ead2b
@taitus
Rename sdg_related_list to related_sdg_list
7fa594e 
To maintain consistency with the current names in the database with fields:
:related_sdg_type and :related_sdg_id
@taitus taitus force-pushed the add_local_target_to_related_list_selector branch from cd5fbb9 to 7fa594e Compare January 26, 2021 18:18
@javierm javierm moved this from Doing to Reviewing in Consul Democracy Jan 26, 2021
Consul Democracy automation moved this from Reviewing to Testing Jan 26, 2021
@javierm javierm merged commit b8cd758 into master Jan 26, 2021
Consul Democracy automation moved this from Testing to Release 1.3.0 Jan 26, 2021
@javierm javierm deleted the add_local_target_to_related_list_selector branch January 26, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierm javierm javierm approved these changes

Assignees

@taitus taitus

@javierm javierm

Labels
SDG United Nations Sustainable Development Goals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@taitus @javierm