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.

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

Custom fields on legislation proposals #3354

Conversation

jaflutz
Copy link
Collaborator

@jaflutz jaflutz commented Mar 13, 2019

References

Closes #3184 - Custom fields on legislation proposals

Objectives

Enables admins to customize the fields of legislation proposals. It makes possible to select which fields are shown to the users and to change the label of the fields.

Visual Changes

Changes in the admin view:

admin_view1

admin_view2

Changes in the users view:

view

spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
@microweb10
Copy link
Member

Hi @jaflutz, thank you very much for this PR!!! 😍 🎉

First of all, my apologies for the late feedback 🙏
Then, just a couple of things:

  • For consistency with the rest of the code, we have introduced some rubocop rules and we use double quotes everywhere, could you please check the Hound alerts and update them using double quotes? 🙏
  • Correct me if I’m wrong, but if you add default: true to the first migration, the second one is not needed ☺️
  • Could you use let instead of let! here? 🙏
  • We normally use build for model validations, instead of create, could you use build here and here? ☺️
  • We prefer to define these kinds of methods in the model when possible.
    Would you mind to try to define them in the model? 🙏 Maybe something like:
def title_label
  self.title_label || self.title_label_value
end
  • I think these checked attributes should not be necessary. Rails should be smart enough to take the value according to the input name. Could you try to delete them and check if it still works as expected? 🙏

You may also want to do a rebase with master since we have introduced a lot of code and now there are some conflicts in order to merge the code in the master branch. 🙂

Please let me know what do you think and if you have any concern about this ☺️

@jaflutz
Copy link
Collaborator Author

jaflutz commented May 14, 2019

Hi @microweb10!
So sorry for so many amateur mistakes 😳
I'll try to get them fixed and rebase this PR as soon as I can!

spec/models/poll/ballot_spec.rb Outdated Show resolved Hide resolved
spec/models/poll/ballot_spec.rb Outdated Show resolved Hide resolved
spec/models/poll/ballot_spec.rb Outdated Show resolved Hide resolved
spec/models/poll/ballot_spec.rb Outdated Show resolved Hide resolved
spec/models/poll/ballot_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
@jaflutz jaflutz force-pushed the 3184-custom-fields-legislation-proposal branch from da67c21 to 09a030c Compare May 22, 2019 00:25
spec/factories/legislations.rb Outdated Show resolved Hide resolved
spec/factories/legislations.rb Outdated Show resolved Hide resolved
spec/factories/legislations.rb Outdated Show resolved Hide resolved
spec/factories/legislations.rb Outdated Show resolved Hide resolved
spec/factories/legislations.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
spec/features/admin/legislation/processes_spec.rb Outdated Show resolved Hide resolved
@jaflutz jaflutz force-pushed the 3184-custom-fields-legislation-proposal branch from 09a030c to ec0a87e Compare May 23, 2019 13:16
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
db/dev_seeds/legislation_processes.rb Outdated Show resolved Hide resolved
app/controllers/admin/legislation/processes_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin/legislation/processes_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin/legislation/processes_controller.rb Outdated Show resolved Hide resolved
…lds, selecting which fields are shown to users and changing its labels.
@jaflutz jaflutz force-pushed the 3184-custom-fields-legislation-proposal branch from ec0a87e to 52408b3 Compare May 23, 2019 15:33
@jaflutz
Copy link
Collaborator Author

jaflutz commented May 23, 2019

Strange, I don't see these errors in travis-ci from my fork. 😕 It seems they're not related with the changes in this PR. Or am I wrong? Any ideas?
Thanks!

@javierm
Copy link
Member

javierm commented May 24, 2019

@jaflutz They're not related to this pull request. Sorry! 🙏. We think we know the cause; we're working on fixing it 😄.

@javierm
Copy link
Member

javierm commented May 24, 2019

@jaflutz Fixed by #3534 😄.

@jaflutz
Copy link
Collaborator Author

jaflutz commented May 24, 2019

Oh, great! 😌
I've just realized I don't know how to fire up travis-ci without pushing again 😬
Thanks @javierm !

@javierm javierm added this to Reviewing in Roadmap Sep 10, 2019
@javierm javierm added this to Reviewing in Consul Democracy Mar 11, 2020
@javierm
Copy link
Member

javierm commented Dec 3, 2021

@jaflutz Very sorry for the late reply! 🙏 And very sorry we're actually closing this pull request 😬

Before doing so, I'd like to really thank you for this pull request, for adding tests, and for taking the time to follow our code conventions. Really appreciate it, and that's why I'm really sorry to close the pull request.

The thing is, we haven't worked on legislation processes so much during the last two years and a half and are currently wondering what to do about this section. Until we reach a decision (which, given our current pace, could take quite some time), we aren't adding this feature regarding custom fileds.

Thank you very much and, once again, sorry. All the best!

@javierm javierm closed this Dec 3, 2021
@javierm javierm removed this from Reviewing in Consul Democracy Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom fields on legislation proposals
4 participants