Skip to content

Add new fields to meetings registration#3123

Merged
josepjaume merged 8 commits intomasterfrom
3044_manage_registrations
Apr 5, 2018
Merged

Add new fields to meetings registration#3123
josepjaume merged 8 commits intomasterfrom
3044_manage_registrations

Conversation

@isaacmg410
Copy link
Copy Markdown
Contributor

🎩 What? Why?

This PR adds new fields for meetings registrations.

Field to reserve specific number of slots
Default reservations terms on component.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add migration to add new field to reserve specific number of slots
  • Add new fields to form and validatations
  • Add new field to component
  • Add translations
  • Add specs

📷 Screenshots (optional)

Description

@ghost ghost assigned isaacmg410 Apr 3, 2018
@ghost ghost added the status: WIP label Apr 3, 2018
@xabier xabier mentioned this pull request Apr 3, 2018
16 tasks
@isaacmg410
Copy link
Copy Markdown
Contributor Author

@decidim/lot-core can you review it?

@mrcasals mrcasals changed the title 3044 manage registrations Add new fields to meetings registration Apr 3, 2018
const toggleDisabledFields = () => {
const enabled = $registrationsEnabled.prop("checked");
$availableSlots.attr("disabled", !enabled);
$reservedSlots.attr("disabled", !enabled);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's this !enabled part? What's enabled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

!enabled is equal to disabled
enabled is based on the registrations_enabled checkbox. If this checkbox, is not checked, admin can't not modifiy the existing fieldavailable_slots and now, we are adding the field reserved_slots with the same functionality.

Is this alright?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOL I didn't see it's a variable declared two lines above 🤦‍♂️ sorry 😅

validates :reserved_slots, numericality: { greater_than_or_equal_to: 0 }, if: ->(form) { form.registrations_enabled? }
validates :reserved_slots, numericality: { less_than_or_equal_to: :available_slots }, if: ->(form) { form.registrations_enabled? }

validate :available_slots_greater_than_or_equal_to_registrations_count, if: ->(form) { form.registrations_enabled? && form.available_slots.positive? }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The if part should be covered by previous validations, right? Should we drop it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

are you sure? This validation existed, I just moved to other line

mrcasals
mrcasals previously approved these changes Apr 3, 2018
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 3, 2018

@isaacmg410 tests are failing

@isaacmg410
Copy link
Copy Markdown
Contributor Author

@mrcasals I will check now

@isaacmg410
Copy link
Copy Markdown
Contributor Author

@mrcasals why codeclimate says Code Climate was unable to fetch this commit. ?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 3, 2018

I don't know, try to rebase against master or pushing an empty commit

@isaacmg410 isaacmg410 force-pushed the 3044_manage_registrations branch from 4a0825a to 592eaa6 Compare April 3, 2018 14:38
@ghost ghost added the status: WIP label Apr 4, 2018
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Apr 4, 2018

@isaacmg410 looks like it's fixed now 😄

@josepjaume josepjaume merged commit d17e1f0 into master Apr 5, 2018
@josepjaume josepjaume deleted the 3044_manage_registrations branch April 5, 2018 08:36
@xabier xabier mentioned this pull request Apr 13, 2018
74 tasks
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.

4 participants