Upgrade to decidim 0.15 #19

tramuntanal · 2018-11-19T16:33:43Z

No description provided.

To confom with the Decidim privacy policy the uploaded document ids are now encrypted.

… of "Municipality Census".

tramuntanal · 2018-11-19T16:35:41Z

Hi @xredo ,
Are you going to keep the maintenance of the module? 🤔
Should be consider it unmantained and stop PRing to it ❓

Thanks! 😄

…xtra backslashes for escaping.

artero · 2018-11-26T10:47:00Z

HI @tramuntanal, I'm Juan from MarsBased. I'm doing this PR review but I have problems testing this branch.

After run:

bin/rails decidim:generate_external_test_app
rspec

I have these errors:

 12) Decidim::FileAuthorizationHandler::Admin::CensusesController POST #delete_all clear all census data
      Failure/Error: 5.times { FactoryBot.create :census_datum, organization: organization }

      ActiveRecord::StatementInvalid:
        PG::UndefinedTable: ERROR:  relation "decidim_file_authorization_handler_census_data" does not exist
        LINE 8:                WHERE a.attrelid = '"decidim_file_authorizati...

This doesn't happen in the master branch.

Looks like as in that Decidim version the gem's migrations don't charge in the dummy app

Do you have the same problem?

tramuntanal · 2018-11-26T14:23:52Z

Yes, its seems that the migration is not automatically copied into the dummy app. We had to manually copy the file:

cp db/migrate/20171110120821_create_decidim_file_authorization_handler_census_datum.rb spec/decidim_dummy_app/db/migrate/

I'll bring this issue to Decidim's main project

artero · 2018-11-26T14:47:59Z

Ok, thank you very much!

artero

Hi @tramuntanal

We have reviewed the code and most of the changes are great. It's nice to have the plugin more configurable and some bugs solved (apart from the upgrade). I would like to apology again in the name of MarsBased for not having performed the reviews before.

We have noticed what it seems to be a bug in the code and we have some doubts that we would like to solve with you before accepting the Pull Request.

Could you please take a look at the comments? Thank you!

artero · 2018-11-26T11:12:54Z

app/permissions/decidim/file_authorizations_handler/admin/permissions.rb

+          return permission_action if permission_action.scope != :admin
+          return Decidim::Proposals::Admin::Permissions.new(user, permission_action, context).permissions if permission_action.scope == :admin
+          if user.organization.available_authorizations.include?("file_authorization_handler")
+            if permission_action_in(:manage, :read)


Should not this be permission_action_in??

decidim-file_authorization_handler.gemspec

artero · 2018-11-26T11:31:53Z

lib/decidim/file_authorization_handler/admin_engine.rb

  module FileAuthorizationHandler
    class AdminEngine < ::Rails::Engine
      isolate_namespace Decidim::FileAuthorizationHandler::Admin
+      paths["db/migrate"] = nil


Why is it required to remove the migrate path?

It was causing apps to crash, I don't remember the exact situation. By the way, what does it mean to set paths["db/migrate"]= nil to not load migrations at runtime?

app/permissions/decidim/file_authorizations_handler/admin/permissions.rb

artero · 2018-11-26T11:36:32Z

app/permissions/decidim/file_authorizations_handler/admin/permissions.rb

+      class Permissions < Decidim::DefaultPermissions
+        def permissions
+          return permission_action if permission_action.scope != :admin
+          return Decidim::Proposals::Admin::Permissions.new(user, permission_action, context).permissions if permission_action.scope == :admin


Why do we need the Proposal permissions here?

I think the lines 12 to 16 never run, because if permission_action.scope != :admin returns line 10 and if not returns line 11.

I rebuilt it.

artero · 2018-11-28T15:24:08Z

app/permissions/decidim/file_authorizations_handler/admin/permissions.rb

+  module FileAuthorizationHandler
+    module Admin
+      # Defines the abilities related to surveys for a logged in admin user.
+      # Intended to be used with `cancancan`.


We to remove this comment

artero · 2018-11-28T15:30:14Z

app/permissions/decidim/file_authorizations_handler/permissions.rb

+    class Permissions < Decidim::DefaultPermissions
+      def permissions
+        return permission_action unless user
+        return Decidim::FileAuthorizationHandler::Admin::Permissions.new(user, permission_action, context).permissions if permission_action.scope == :admin


Maybe I am missing something, but I don't see why we need to check for the Admin permissions in here. Could you explain the use case? Thanks!

I've removed this file as there is no one is using it.

tramuntanal · 2018-12-18T10:18:20Z

Hi @artero sorry I lost notifications and didn't know you made the review.
I'll give you feedback in a week or two.

…dim-file_authorization_handler into upgrade/decidim-0.15

…e deprecation.

…t clear why it is being setted.

tramuntanal · 2019-01-04T11:08:23Z

Hi @artero the PR has been updated as requested.

artero

We can merge, Thankyou very much!

xredo and others added 20 commits March 21, 2018 13:47

Encodes document ids using a SHA256 hex digests

2350d70

To confom with the Decidim privacy policy the uploaded document ids are now encrypted.

Update versions for gem and dependencies.

56292d4

[DOC] Explain verification data is not persisted.

b2b3376

[REFACTOR] Replace old abilities (cancancan) for new permissions system.

a70c6aa

[REFACTOR] Replace old authorize! for the new enforce_permission_to.

986daca

[REFACTOR] Use the standard separator for CSV.

fb1d1e5

Declare the component_permissions.js asset.

6739fb6

[REFACTOR] Apply renaming of feature -> component.

c9b6305

Git ignore: .rspec-failures file.

85a167a

[REFACTOR] Remove useless artifacts.

0b21665

Definitive 0.11.1 version.

a33b86b

[TEST] Adapt test to 0.11 release and ',' separator.

ba7995a

Update Gemfile to 0.11.1 version.

8f29ff9

[REFACTOR] Make CSV separator configurable through initializer.

05beb39

[REFACTOR] Remove debugging puts.

e9e47ef

[REFACTOR] Log which csv separator is being used.

621407d

[FIX] Do not crash when there are no rows to insert.

f3cde9a

Upgrade to Decidim 0.12.

1085cb6

[I18N] Use a more neutral terminology: "Oganization's Census" instead…

ae8669c

… of "Municipality Census".

[CONFIG] Upgrade configuration to Decidim 0.15.

210f16c

Oliver Valls added 3 commits November 19, 2018 17:45

Rubocopify and linterize.

a4a71b1

Prefer double-quoted strings unless you need single quotes to avoid e…

e0e3e54

…xtra backslashes for escaping.

Extra empty line detected at class body beginning.

c975d50

tramuntanal mentioned this pull request Nov 20, 2018

[WIP] Fix/ensure all data properly encoded #17

Closed

5 tasks

tramuntanal mentioned this pull request Nov 26, 2018

External test app generator not copying module's migrations decidim/decidim#4572

Closed

artero suggested changes Nov 28, 2018

View reviewed changes

xredo mentioned this pull request Dec 7, 2018

Added an option to unauthorize all previously authotized users. #18

Open

Isaac Massot and others added 10 commits December 19, 2018 10:37

change Date do Decidim::Attributes::LocalizedDate

41de957

[DOC] Update gem's contact info.

5c725a4

Merge branch 'upgrade/decidim-0.15' of github.com:CodiTramuntana/deci…

a161baa

…dim-file_authorization_handler into upgrade/decidim-0.15

[REFACTOR] Remove deprecated comment.

c388de6

[FIX] Mistyped method name.

01924d3

[REFACTOR] Static attributes will be removed in FactoryBot 5.0, updat…

bd5ee98

…e deprecation.

[REFACTOR] Remove useless permissions file.

56e6902

[REFACTOR] Remove setting Engine's db/migrate path to nil as it is no…

5f3f36a

…t clear why it is being setted.

[REFACTOR/TEST] Rebuild and apply tests to admin permissions.

68bab4b

[DOC] Apply markdown linter requisites.

63fb79d

tramuntanal force-pushed the upgrade/decidim-0.15 branch from 23ea1a3 to 90eb240 Compare December 31, 2018 16:19

More linter complaints.

ebc4388

tramuntanal force-pushed the upgrade/decidim-0.15 branch from 90eb240 to ebc4388 Compare December 31, 2018 16:21

artero approved these changes Jan 14, 2019

View reviewed changes

artero merged commit f01aaa7 into MarsBased:master Jan 14, 2019

tramuntanal deleted the upgrade/decidim-0.15 branch January 14, 2019 12:32

Upgrade to decidim 0.15 #19

Upgrade to decidim 0.15 #19

Uh oh!

Conversation

tramuntanal commented Nov 19, 2018

Uh oh!

tramuntanal commented Nov 19, 2018

Uh oh!

artero commented Nov 26, 2018

Uh oh!

tramuntanal commented Nov 26, 2018

Uh oh!

artero commented Nov 26, 2018

Uh oh!

artero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

tramuntanal commented Dec 18, 2018

Uh oh!

tramuntanal commented Jan 4, 2019

Uh oh!

artero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants