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

Remove duplication from participatory spaces publications controllers #11549

Merged
merged 8 commits into from
Sep 7, 2023
Merged

Remove duplication from participatory spaces publications controllers #11549

merged 8 commits into from
Sep 7, 2023

Conversation

andreslucena
Copy link
Member

@andreslucena andreslucena commented Aug 30, 2023 •
edited
Loading

🎩 What? Why?

CodeClimate found that the publications controller were duplicated. This PR refactors them so they're no longer duplicated.

In the process, I couldn't find the specs for this feature (Publish/Unpublish of ParticipatorySpaces), so I went ahead and add it, just to be sure that I'm not breaking anything related to this feature.

📌 Related Issues

Testing

All the CI should be green

📷 Screenshots

From Codeclimate:

Screenshot of Codeclimate alert

Mind that the link will expire in a couple of weeks.

Also it's mentioning two locations but I found 2 more locations.

♥️ Thank you!

@andreslucena andreslucena marked this pull request as draft August 30, 2023 14:13
@andreslucena andreslucena changed the title Add spec for participatory spaces publications Remove duplication from participatory spaces publications controllers Aug 30, 2023
@andreslucena andreslucena changed the title Remove duplication from participatory spaces publications controllers Remove duplication from participatory spaces publications controllers Aug 30, 2023
@andreslucena andreslucena force-pushed the refactor/participatory-space-publications branch from cb83c0a to c2bf464 Compare August 31, 2023 06:30
@andreslucena andreslucena marked this pull request as ready for review August 31, 2023 07:11
@andreslucena andreslucena requested a review from a team August 31, 2023 07:11
alecslupu
alecslupu requested changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

This is a great change!
I have found some items that i think it could be improved.

decidim-assemblies/app/controllers/decidim/assemblies/admin/assembly_publications_controller.rb Outdated Show resolved Hide resolved
...-conferences/app/controllers/decidim/conferences/admin/conference_publications_controller.rb Outdated Show resolved Hide resolved
decidim-elections/app/controllers/decidim/votings/admin/voting_publications_controller.rb Outdated Show resolved Hide resolved
...llers/decidim/participatory_processes/admin/participatory_process_publications_controller.rb Outdated Show resolved Hide resolved
@alecslupu
Copy link
Contributor

As a side note, i have noticed the Votings Admin Concern does not follow namespace convention.

@alecslupu alecslupu mentioned this pull request Sep 6, 2023
Merged
andreslucena and others added 2 commits September 7, 2023 07:54
@andreslucena @alecslupu
Remove unecessary method for current_participatory_space
5337aa8 
Suggested by code review

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
@andreslucena
Remove empty lines
ecf4278
@andreslucena
Copy link
Member Author

As a side note, i have noticed the Votings Admin Concern does not follow namespace convention.

I understand that you're referring to the file paths, right? Like with decidim-*/app/controllers/decidim/*/admin/concerns/*_admin.rb we get all the concerns for spaces that have the suffix "_admin" except Votings that is on another path (decidim-elections/app/controllers/concerns/decidim/votings/admin/voting_admin.rb). Is that it?

@alecslupu
Copy link
Contributor

As a side note, i have noticed the Votings Admin Concern does not follow namespace convention.

I understand that you're referring to the file paths, right? Like with decidim-*/app/controllers/decidim/*/admin/concerns/*_admin.rb we get all the concerns for spaces that have the suffix "_admin" except Votings that is on another path (decidim-elections/app/controllers/concerns/decidim/votings/admin/voting_admin.rb). Is that it?

I was refering to the fact that all the other concerns are being defined as:

  • Decidim::Assemblies::Admin::Concerns::AssemblyAdmin
  • Decidim::Conferences::Admin::Concerns::ConferenceAdmin
  • Decidim::ParticipatoryProcesses::AdminConcerns::ParticipatoryProcessAdmin

Yet, the admin is being defined as:

  • Decidim::Votings::Admin::VotingAdmin

So yeah, in the end is about the place where the file is being saved.

alecslupu
alecslupu approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

LGTM

@alecslupu alecslupu added this to the 0.28.0 milestone Sep 7, 2023
@alecslupu alecslupu merged commit 08f506b into develop Sep 7, 2023
81 checks passed
@alecslupu alecslupu deleted the refactor/participatory-space-publications branch September 7, 2023 08:04
entantoencuanto added a commit that referenced this pull request Sep 8, 2023
@entantoencuanto
Merge branch 'fix/activities-block-follow-button' into feature/redesi…
e9421e8 
…gn-staging

* fix/activities-block-follow-button: (27 commits)
  Add tests to follow button in processes and assemblies landing page
  Add follow button to participatory spaces last activities content block
  Remove duplication from participatory spaces publications controllers (#11549)
  Fix the a11y tool icons with redesign (#11175)
  Remove duplication from amendments events specs (#11553)
  Remove duplication from elections' user roles forms (#11548)
  Update Node.js from v16.13.0 to v18.17.1 (#11564)
  Remove duplication from stats presenters (#11551)
  Fix Bootsnap configuration (#11483)
  Remove duplication for add questions specs examples (#11559)
  Remove duplication from invites queries (#11552)
  Fix typos and copy-paste errors from comments and examples (#11536)
  Fix conference venues meetings visibility (#11542)
  Add recognition to BrowserStack in the README (#11546)
  Remove unused view hook for `:upcoming_meeting_for_card` (#11543)
  Remove unused dependency: `wicked` (#11150)
  Clean-up initiatives signature URLs and methods (#11545)
  Refactor initiative signing wizard (#10731)
  Fix Permissions screen on budgets throw errors (#11532)
  Redesign: read more literal (#11516)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecslupu alecslupu alecslupu approved these changes

Assignees
No one assigned
Labels
module: admin module: assemblies module: conferences module: elections module: participatory processes type: internal PRs that aren't necessary to add to the CHANGELOG for implementers
Projects
Maintainers' backlog
Archived in project
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andreslucena @alecslupu