Improve the budget page and the project card by agustibr · Pull Request #5809 · decidim/decidim

agustibr · 2020-02-28T08:29:33Z

🎩 What? Why?

Admin can manage project images.
Project card and page, show images.
Project card with description

📌 Related Issues

Fixes Improve the budget cards and the budget page #5710

📋 Subtasks

Add CHANGELOG entry
Add tests
Extracted Attachment and Gallery Methods from decidim-proposals to decidim-core so they can be used in decidim-budgets, and share the functionality.

📷 Screenshots (optional)

Admin can add images to projects:

- Project cards with image and description:

Project details with all images:

tramuntanal · 2020-04-01T08:32:50Z

Hi @agustibr is this still a draft? or it is ready to merge?

agustibr · 2020-04-01T08:45:02Z

Hi @tramuntanal ! it's a draft as the design part was pending.

agustibr · 2020-04-21T06:40:35Z

@decidim/core this is approved by @decidim/product , can you review it please?

tramuntanal

Hi @agustibr ,
Good job!

I've left a few suggestions, and have the following doubt:
Why do we have a create_dummy_resource (I can't see where it is used) and the corresponding create_dummy_resource_spec?

tramuntanal · 2020-04-27T06:22:07Z

CHANGELOG.md

 - **decidim-core**: Fix clearing the current_user after sign out [#5823](https://github.com/decidim/decidim/pull/5823)
 - **decidim-budgets**: Send email with summary on order checkout [\#6006](https://github.com/decidim/decidim/pull/6006)
 - **decidim-budgets**: Projects filter by multiple categories [/#5992](https://github.com/decidim/decidim/pull/5992)
+- **decidim-budgets**: Projects filter by multiple categories [\#5992](https://github.com/decidim/decidim/pull/5992)


It seems you doubled the "Projects filter by multiple categories" entry by mistake when fixing the slash

tramuntanal · 2020-04-27T07:45:16Z

decidim-core/app/commands/decidim/gallery_methods.rb

+      end
+      # manually reset cached photos
+      @attached_to.reload
+      @attached_to.instance_variable_set(:@photos, nil)


why do we need instance_variable_set and not use a regular accessor? we should avoid metaprograming..

I moved the existing gallery_methods.rb (and attachment_methods.rb) from decidim-proposals to decidim-core the author is @microstudi, surely he can answer you better than I 😁

decidim/decidim-proposals/app/commands/decidim/proposals/gallery_methods.rb

Line 54 in d853ef1

@attached_to.instance_variable_set(:@photos, nil)

To be honest, I don't remember 😓 . Probably I was testing something and it wasn't spotted at the review. looking at the code now, it certainly doesn't seem necessary at all.

Thanks @microstudi for the answer 😃

tramuntanal · 2020-04-27T08:05:23Z

decidim-proposals/app/commands/decidim/proposals/update_proposal.rb

    # A command with all the business logic when a user updates a proposal.
    class UpdateProposal < Rectify::Command
-      include AttachmentMethods
+      include Decidim::AttachmentMethods


Suggested change

include Decidim::AttachmentMethods

include ::Decidim::AttachmentMethods

tramuntanal · 2020-04-27T08:06:54Z

decidim-proposals/app/commands/decidim/proposals/admin/create_proposal.rb

      # A command with all the business logic when a user creates a new proposal.
      class CreateProposal < Rectify::Command
-        include AttachmentMethods
+        include Decidim::AttachmentMethods


In modules it is typical to have problems with namespacing. If we use the absolute namespace this will avoid having to perform many overrides

Suggested change

include Decidim::AttachmentMethods

include ::Decidim::AttachmentMethods

The reason is that this is not widely used in Decidim, but yes I'll change it to use the absolute namespace.

tramuntanal · 2020-04-27T08:07:03Z

decidim-proposals/app/commands/decidim/proposals/admin/update_proposal.rb

      # A command with all the business logic when a user updates a proposal.
      class UpdateProposal < Rectify::Command
-        include AttachmentMethods
+        include Decidim::AttachmentMethods


Suggested change

include Decidim::AttachmentMethods

include ::Decidim::AttachmentMethods

tramuntanal · 2020-04-27T08:07:13Z

decidim-proposals/app/commands/decidim/proposals/create_collaborative_draft.rb

    # A command with all the business logic when a user creates a new collaborative draft.
    class CreateCollaborativeDraft < Rectify::Command
-      include AttachmentMethods
+      include Decidim::AttachmentMethods


Suggested change

include Decidim::AttachmentMethods

include ::Decidim::AttachmentMethods

tramuntanal · 2020-04-27T08:07:23Z

decidim-proposals/app/commands/decidim/proposals/create_proposal.rb

    # A command with all the business logic when a user creates a new proposal.
    class CreateProposal < Rectify::Command
-      include AttachmentMethods
+      include Decidim::AttachmentMethods


Suggested change

include Decidim::AttachmentMethods

include ::Decidim::AttachmentMethods

agustibr · 2020-04-27T10:48:44Z

I've left a few suggestions, and have the following doubt:
Why do we have a create_dummy_resource (I can't see where it is used) and the corresponding create_dummy_resource_spec?

Hi @tramuntanal, as far as I know, codecov only takes into account tests from each module independly (without taking into account if it's tested in another Decidim module).

In this PR Decidim::AttachmentMethods and Decidim::GalleryMethods are part of decidim-core so its pourpose is to test the methods within the module.

The create_dummy_resource.rb is been tested in the corresponding create_dummy_resource_spec.rb.

😁

tramuntanal · 2020-04-28T06:42:24Z

Hi @agustibr , thanks for applying the suggestions.

In my question I was asking why create a create_dummy_resource command, not the placement of its tests. Probably I'm missing something and we're misunderstanding each other 🤦
So, what I see is we're creating a command for a dummy resource and testing this command, but this command is not used anywhere else. Are we indirectly testing something that can't be tested with "production" code? why do we need the CreateDummyResource command?

agustibr · 2020-04-28T09:21:04Z

Hi @tramuntanal ,
In production Decidim::AttachmentMethods and Decidim::GalleryMethods methods are used (Included) in projects and proposals commands when creating/updating the resources.

The CreateDummyResource and its spec are for coverage of Decidim::AttachmentMethods and Decidim::GalleryMethods, as is required that each module has to have its own code covered with its own test to be counted as tested/covered in codecov reports, so as this methods are for resources I used the DummyResource present in decidim-core.

I also added this methods tests in Budgets::Project command specs, as there are in Proposals.

Do you have any thoughts on how to test it in another way?

tramuntanal · 2020-04-29T06:18:42Z

Ok @agustibr lets keep CreateDummyResource and its spec then, but if you can extract reusable examples and apply them in proposals and budgets it would be great. Like "global search of participatory spaces" in decidim-core/lib/decidim/core/test/shared_examples/searchable_participatory_space_examples.rb is being included in all searchable spaces specs.

agustibr · 2020-04-30T06:16:00Z

@tramuntanal I've extracted it and applied them in proposals and budgets. This should be ready to merge now 😁

tramuntanal · 2020-04-30T06:16:08Z

@agustibr there's a test related to translations failing. I guess you can solve it with bin/rake webpack

tramuntanal · 2020-04-30T06:17:01Z

well, I'll try to manage it while merging

agustibr · 2020-04-30T06:17:48Z

Ok, thanks @tramuntanal 👍

* Fix Js bundle sanity * add admin resource gallery examples * Apply suggested changes by decidim/core * fix stylelint issues * refix specs * Add decidm-core specs for GalleryMethods and AttachmentMethods. * padding when project card has no image * Update Changelog * feat(budgets): create ProjectListItemCell * move AttachmentMethods, GalleryMethods and related out of Proposals to use it in Budgets * Project's card with image and description * Admin can manage image gallery in projects

tramuntanal · 2020-04-30T06:51:00Z

Squashed and merged to develop via 0ecdb3027f532501fce4339157d1e9c5e440d63b. Github does not automatically detects so I'm manually closing this PR.

* develop: (29 commits) Update Conversations design with decidim-design UI (decidim#6008) Add counter of active users to admin dashboard (decidim#5907) Show activity graphs on admin dashboard (decidim#6030) Update sassc gem version (decidim#6062) Fix generator Gemfile after puma upgrade (decidim#6060) New Crowdin translations (decidim#6059) Add Slovak as a new language (decidim#6039) Remove all tests for i18n PRs (decidim#6061) Update move up and down buttons after dragging questions when managing questionnaire (decidim#5947) Fix using Decidim as a provider for omniauth authentication (decidim#6042) Add redesign for responsive public profile navigation tabs (decidim#6032) Add versioning pages to initiatives (decidim#5935) Notify users when roles get assigned (decidim#5886) Improve the budget page and the project card (decidim#5809) New Crowdin translations (decidim#6050) New Crowdin translations (decidim#6046) Ignore jobs on locales branches (decidim#6047) Automatic task for deleting Meeting Inscription data (decidim#5989) New Crowdin translations (decidim#5877) Ignore builds on Crowdin PRs (decidim#6037) ...

agustibr mentioned this pull request Feb 28, 2020

Improve the budget cards and the budget page #5710

Closed

4 tasks

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch 2 times, most recently from 26db64f to 921d50d Compare March 3, 2020 08:36

leio10 mentioned this pull request Apr 3, 2020

Budgets component improvement #5925

Closed

3 tasks

agustibr changed the base branch from develop to feat/complex-budgets-voting April 3, 2020 13:42

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from 921d50d to a83fed6 Compare April 6, 2020 15:04

agustibr changed the base branch from feat/complex-budgets-voting to develop April 6, 2020 15:06

agustibr added project: PAM2020 Barcelona City Council contract status: WIP labels Apr 6, 2020

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from a83fed6 to c065da5 Compare April 6, 2020 15:08

agustibr marked this pull request as ready for review April 6, 2020 20:41

agustibr removed the status: WIP label Apr 7, 2020

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from 4d378de to 66dc6e1 Compare April 7, 2020 06:52

agustibr requested a review from mrcasals April 7, 2020 07:29

mrcasals previously approved these changes Apr 7, 2020

View reviewed changes

agustibr dismissed mrcasals’s stale review via fdcf44a April 8, 2020 15:52

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from 66dc6e1 to fdcf44a Compare April 8, 2020 15:52

agustibr self-assigned this Apr 8, 2020

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch 4 times, most recently from 536602f to 54202b6 Compare April 13, 2020 11:06

mrcasals previously approved these changes Apr 14, 2020

View reviewed changes

agustibr dismissed mrcasals’s stale review via efc076f April 20, 2020 13:56

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from 136db22 to efc076f Compare April 20, 2020 13:56

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from 00bd150 to 602ee3a Compare April 22, 2020 06:39

Admin can manage image gallery in projects

d8209f0

agustibr added 4 commits April 24, 2020 08:48

padding when project card has no image

852b7a7

fix specs

76a622e

Add decidm-core specs for GalleryMethods and AttachmentMethods.

c56b494

update changelog

bf709e5

agustibr force-pushed the feature/improve-budget-page-and-project-cards branch from 7f7aae9 to bf709e5 Compare April 24, 2020 07:04

refix specs

af19a63

tramuntanal added the in-review label Apr 27, 2020

tramuntanal self-assigned this Apr 27, 2020

fix stylelint issues

53c0541

tramuntanal suggested changes Apr 27, 2020

View reviewed changes

Apply suggested changes by decidim/core

a792dfe

agustibr requested a review from tramuntanal April 27, 2020 12:55

add admin resource gallery examples

5bcd0bd

tramuntanal approved these changes Apr 30, 2020

View reviewed changes

tramuntanal closed this Apr 30, 2020

tramuntanal deleted the feature/improve-budget-page-and-project-cards branch April 30, 2020 06:51

ace mentioned this pull request May 29, 2020

Fixes including the initiatives module instead of the core one. #6140

Merged

1 task

mrcasals mentioned this pull request Dec 21, 2020

Allow selecting multiple files on gallery forms #7052

Merged

	include Decidim::AttachmentMethods
	include ::Decidim::AttachmentMethods

Uh oh!

Conversation

agustibr commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

tramuntanal commented Apr 1, 2020

Uh oh!

agustibr commented Apr 1, 2020

Uh oh!

agustibr commented Apr 21, 2020

Uh oh!

tramuntanal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agustibr commented Apr 27, 2020

Uh oh!

tramuntanal commented Apr 28, 2020

Uh oh!

agustibr commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tramuntanal commented Apr 29, 2020

Uh oh!

agustibr commented Apr 30, 2020

Uh oh!

tramuntanal commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tramuntanal commented Apr 30, 2020

Uh oh!

agustibr commented Apr 30, 2020

Uh oh!

tramuntanal commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agustibr commented Feb 28, 2020 •

edited

Loading

agustibr commented Apr 28, 2020 •

edited

Loading

tramuntanal commented Apr 30, 2020 •

edited

Loading