-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
Remove Participation module #3909
Conversation
Hi!, @ferblape about gobierto/test/models/gobierto_cms/page_test.rb Lines 62 to 70 in 91e9cfe
about this test this should be (2 preferred)
|
You can remove that test and the method destroyable? in cms page model
…On Thu, 1 Jul 2021 at 10:00, stbnrivas ***@***.***> wrote:
Hi!, @ferblape <https://github.com/ferblape> about
https://github.com/PopulateTools/gobierto/blob/91e9cfee11ab6bb55990e9ed38dae1a5612e6391/test/models/gobierto_cms/page_test.rb#L62-L70
about this test this should be (2 preferred)
1. removed
2. replaced for another model ¿why model can I use?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3909 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEJUBNVHQMIQI7QBWNTTLTVQOCPANCNFSM47T5TJ7Q>
.
--
Fernando Blat
***@***.***
+34 660825001
Populate / Tools for civic engagement
https://populate.tools
Project stories
twitter.com/populate_ & populate.tools/blog
|
what's about gobierto/app/controllers/gobierto_attachments/attachment_documents_controller.rb Lines 32 to 41 in 91e9cfe
thx |
Yes, they are used in the cms page model.
Remove the assignment to the current process in that method and leave the
rest
…On Thu, 1 Jul 2021 at 10:33, stbnrivas ***@***.***> wrote:
what's about attachment? Does it make sense without
GobiertoParticipations::Process?
https://github.com/PopulateTools/gobierto/blob/91e9cfee11ab6bb55990e9ed38dae1a5612e6391/app/controllers/gobierto_attachments/attachment_documents_controller.rb#L32-L41
thx
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3909 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEJUCWT4AG345BTKWGGPLTVQR7HANCNFSM47T5TJ7Q>
.
--
Fernando Blat
***@***.***
+34 660825001
Populate / Tools for civic engagement
https://populate.tools
Project stories
twitter.com/populate_ & populate.tools/blog
|
cd5b708
to
cc0082d
Compare
@blat I have a problem with a test that before works and not now ... I am missing something... gobierto/test/models/gobierto_common/term_test.rb Lines 10 to 24 in 91e9cfe
but the problem is that this fixtures(culture_term, center_term, marvel_term) don't have dependent into sublevel gobierto/test/models/gobierto_common/term_test.rb Lines 71 to 75 in 91e9cfe
an example of dependant level with animals and dog fixtures level is (note line 19 below) gobierto/test/fixtures/gobierto_common/terms.yml Lines 3 to 20 in 91e9cfe
but into all term.yml there are single culture_term
# change this
def test_destroy_of_term_with_dependencies
terms_with_dependencies.each do |_, term|
refute term.destroy
end
end
# to this
def test_destroy_of_term_with_dependencies
refute root_level_term.destroy
end But I have a worse problem the decorator don't detect terms collection here gobierto/app/models/gobierto_common/term.rb Lines 45 to 53 in 91e9cfe
TermDependendentResourceDecorator is not enough. I need add some changes at: gobierto/app/models/gobierto_common/term.rb Lines 45 to 53 in 91e9cfe
What am I missing? |
cc0082d
to
9bb7fc7
Compare
should I delete (full) or refactor this decorator and its test? . I am not sure about because it looks like a feature into GobiertoCMS designed to be used by GobiertoParticipations. So with participation deleted. Does it keeping sense? gobierto/test/decorators/gobierto_cms/page_decorator_test.rb Lines 27 to 33 in 91e9cfe
thx |
Hmmmm, not exactly, that decorator is used to customize GobiertoCms::Pages, which are used in participation module but also in other modules. So leave the decorator and update the test to remove the relative to themes. |
what about this test? should I delete it or where should I redirect path instead of gobierto/test/integration/gobierto_admin/gobierto_common/update_cms_pages_collection_test.rb Lines 5 to 47 in 91e9cfe
thx |
Delete it, please
…On Fri, 2 Jul 2021 at 16:49, stbnrivas ***@***.***> wrote:
what about this test? should I delete it or where should I redirect path
instead of admin_participation_process_pages_path(process.id)
https://github.com/PopulateTools/gobierto/blob/91e9cfee11ab6bb55990e9ed38dae1a5612e6391/test/integration/gobierto_admin/gobierto_common/update_cms_pages_collection_test.rb#L5-L47
thx
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3909 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEJUBN4URFWDEJYR3ANQDTVXGZHANCNFSM47T5TJ7Q>
.
--
Fernando Blat
***@***.***
+34 660825001
Populate / Tools for civic engagement
https://populate.tools
Project stories
twitter.com/populate_ & populate.tools/blog
|
hi, just to confirm, the test into this folder https://github.com/PopulateTools/gobierto/tree/master/test/integration/gobierto_admin/gobierto_attachments are dependent of some fixtures of GobiertoParticipations. Should I refactor or delete? thx |
9bb7fc7
to
8c0344b
Compare
refactor, gobierto attachments are out of the affected modules
…On Mon, 5 Jul 2021 at 16:42, stbnrivas ***@***.***> wrote:
hi, just to confirm the test into this folder
https://github.com/PopulateTools/gobierto/tree/master/test/integration/gobierto_admin/gobierto_attachments
are dependent of some fixtures of GobiertoParticipations.
Should I refactor or delete?
thx
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3909 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEJUHRHC63IXZ4BFYKPJLTWHAG5ANCNFSM47T5TJ7Q>
.
--
Fernando Blat
***@***.***
+34 660825001
Populate / Tools for civic engagement
https://populate.tools
Project stories
twitter.com/populate_ & populate.tools/blog
|
gobierto/db/migrate/20170719091606_add_type_to_processes.rb Lines 3 to 7 in 91e9cfe
After the module deletion the class
Just to say, there are 4 cases of migrations like this Thx |
Good point, using |
Should join fixtures and fixtures_archive? at least which that are still in use ... or I'm missing something? gobierto/test/fixtures/gobierto_cms/pages.yml Lines 1 to 4 in 91e9cfe
thx! |
So attachment and participation have a set of test cases which I need remove or rewrite as new feature. Some details: gobierto/test/integration/gobierto_admin/gobierto_attachments/file_attachment_index_test.rb Line 40 in 91e9cfe
gobierto/test/integration/gobierto_admin/gobierto_common/collections/edit_collection_test.rb Line 46 in 91e9cfe
gobierto/test/integration/gobierto_admin/gobierto_common/collections/show_collection_test.rb Line 35 in 91e9cfe
gobierto/test/integration/gobierto_admin/gobierto_common/collections/show_collection_test.rb Line 46 in 91e9cfe
before deletion module attachment show the collection and attachments that belongs to I think that this behavior don't apply to attachment collections which don't belongs to Process:
therefore this test should be removed, or I am missing something? |
What I'd do is to update the collections of the tests to different ones. In
test/fixtures/gobierto_common/collections.yml you have more available, you
could use site_pages, site_news (if you want to use the site Madrid)
…On Wed, 7 Jul 2021 at 12:42, stbnrivas ***@***.***> wrote:
So attachment and participation have a set of test cases which I need
remove or rewrite as new feature. Some details:
https://github.com/PopulateTools/gobierto/blob/91e9cfee11ab6bb55990e9ed38dae1a5612e6391/test/integration/gobierto_admin/gobierto_attachments/file_attachment_index_test.rb#L40
https://github.com/PopulateTools/gobierto/blob/91e9cfee11ab6bb55990e9ed38dae1a5612e6391/test/integration/gobierto_admin/gobierto_common/collections/edit_collection_test.rb#L46
before deletion module attachment show the collection and attachments that
belongs to GobiertoParticipation::Process. These process can be archived
with paranoid and then these collections were not show.
I think that this behavior don't apply to attachment collections which
don't belongs to Process:
- I searched examples into staging admin/attachments/file_attachments
- then archive all attachment into a collection
- the collection it keep showing (I think that it's right)
therefore this test should be removed, or I am missing something?
thx
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3909 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEJUHVKEI5WPQVUOFBJBLTWQVSRANCNFSM47T5TJ7Q>
.
--
Fernando Blat
***@***.***
+34 660825001
Populate / Tools for civic engagement
https://populate.tools
Project stories
twitter.com/populate_ & populate.tools/blog
|
that was my first step. OkI will separate into different commit to speak about later thx |
7426721
to
53f5b08
Compare
fb6ff9f
to
aa050ed
Compare
@entantoencuanto please continue the work of Esteban in this PR, he's commenting about some errors in gencat engine tests, but it's weird because participation and gencat are not related and don't share code. |
87d04a4
to
0d6b623
Compare
The class with the module name is used by some engines to select elements of the DOM
0d6b623
to
dd0500e
Compare
53a7879
to
04fca8c
Compare
Closes #3826
✌️ What does this PR do?
remove module participations and its dependencies into other modules