Skip to content

Meetings: Registration form#3331

Closed
rbngzlv wants to merge 6 commits intomasterfrom
feature/meetings_registration_form
Closed

Meetings: Registration form#3331
rbngzlv wants to merge 6 commits intomasterfrom
feature/meetings_registration_form

Conversation

@rbngzlv
Copy link
Copy Markdown
Contributor

@rbngzlv rbngzlv commented May 3, 2018

🎩 What? Why?

Meeting can have a registration form to ask questions to the participants.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add tests

📷 Screenshots (optional)

@ghost ghost assigned rbngzlv May 3, 2018
@ghost ghost added the status: WIP label May 3, 2018
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented May 3, 2018

As we commented in #3044 (comment) I'm trying to reuse the surveys component inside another component (meetings).

At the moment @decidim/product have two use cases:

  • Add a form to the registration process
  • Send a survey at the end of the meeting

In this PR I'm working in one possible approach: rename participatory_space to part_of and reuse the actual component entity, but I'm not sure about this.

I have also been testing the creation of a new entity called subcomponent and adding a configuration to the component_manifest to configure what components (as subcomponents) are supported in this component. With this approach we'll be creating a new table, controllers, views, etc. very similar to the current components but perhaps is more maintainable.

@decidim/lot-core can we discuss the best way to solve this, as the decisions can be very important for another features in the future.

@decidim/lot-mods ideas on this are welcome.

@xabier
Copy link
Copy Markdown

xabier commented May 4, 2018

@rbngzlv functionally speaking I very much like your approach. It is great!. I am curious as to how @decidim/developers and @decidim/lot-core see it.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented May 4, 2018

I know this would imply a big refactor, but I'd personally go with the part_of refactor. This would allow us to plug components to other places (like a user profile, which has already been discussed offline).

Let's wait until we hear some other opinions, though!

@xabier
Copy link
Copy Markdown

xabier commented May 4, 2018

@mrcasals thanks for pointing this out:

This would allow us to plug components to other places (like a user profile, which has already been discussed offline).

This is certainly very useful. On top of being capable to activate components in user-profile pages, there are also other use cases, like calling for a meeting that is global to the platform (e.g. special meeting, training courses on using the platform, etc.). Another use case is to activate a component like the blog also for the whole platform (as part of the top menu), or to use the debates components as a user-forum... I am pointing out this use cases to help on making the technical decisions about the platform architecture. These are some of the demands that we receive as product-coordinators and we consider to be of high value for Decidim. Hope it helps for the architectural decision.

@andreslucena any thoughts?

@oriolgual
Copy link
Copy Markdown
Contributor

I'd be very much against reusing the surveys component for these two features. It would create a lot of coupling and a lot of work to be able to use a component inside of another component (and probably a lot of bugs afterwards).

I rather create these two specific features at the meetings component than trying to reuse the surveys component inside another one.

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented May 9, 2018

@decidim/lot-core and @decidim/product can you decide and clarify what approach we have to follow to implement this?

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

For what it's worth I share the same feeling with @oriolgual. My gut tells me this is the wrong abstraction and it feels better to live with some (or a lot of) code duplication rather than opening this door now.

Maybe surveys could be "promoted" to be something more general than a component that can be reused anywhere. After all, they are already a special component in the sense that they are "singletons" (a single survey is allowed on each survey component). That "singleton restriction" could be easily removed but the fact that we needed to add it might be a sign that a "component" is not the best fit for them. Maybe "pages" are in a similar situation.

Not sure, just throwing out some thoughts.

@josepjaume
Copy link
Copy Markdown
Contributor

josepjaume commented May 9, 2018

I share the exact same thoughts as @deivid-rodriguez and @oriolgual. Componetization and composition at this level isn't possible with the current architecture and achieving something like this would probably mean reinventing a whole framework (it vastly exceeds rails' capabilities and the project scope).

I think duplicating part of the code would be more maintainable and easier to reason about, also less error-prone. It also forces us to think in the specific use case instead of invoking the "more flexibility is always good" mantra.

@xabier xabier mentioned this pull request May 18, 2018
16 tasks
@rbngzlv rbngzlv force-pushed the feature/meetings_registration_form branch 3 times, most recently from 8233284 to 3d2d96e Compare May 25, 2018 10:12
@rbngzlv rbngzlv force-pushed the feature/meetings_registration_form branch 4 times, most recently from fc675ff to 5015b91 Compare May 27, 2018 15:45
@rbngzlv rbngzlv force-pushed the feature/meetings_registration_form branch from 5015b91 to 6db138b Compare May 27, 2018 18:08
@rbngzlv rbngzlv force-pushed the feature/meetings_registration_form branch from 6db138b to cf87696 Compare May 27, 2018 18:08
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented May 28, 2018

@decidim/lot-core ready to review!

@xabier xabier mentioned this pull request May 28, 2018
74 tasks
@josepjaume
Copy link
Copy Markdown
Contributor

josepjaume commented Jun 4, 2018

Good job! This is a fair amount of work!

That said, don't you feel like maybe we added too much complexity? The amount of code added seems to indicate the feature could have been defined in simpler terms, maybe with a predefined, opinionated questionnaire instead of entirely replicating the highly complex surveys module. I know I'm late with this and won't block it, but it really feels like we got a bit too far.

In any case, I recognize the amount of work and effort put into this and in any case would like it to seem like I'm dismissing it. It's just that whenever we add something this big, we should consider long-term implications and maintenance.

@rbngzlv - is the entire surveys functionality replicated here, or just a part? What's the example use case behind this?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Jun 4, 2018

I still think duplicating the whole feature is not the best way to go. This will force us keep two libraries that do the same, but might potentially not be 100% equal. Bugs appearing to the surveys component will need to be ported to the meetings part, etc, and this is a big burden.

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Jun 4, 2018

My first proposal was to reuse the surveys component, as a soft dependency of the meetings component, but you rejected it in favor of duplicating code within the meetings component. It was clear that this had other disadvantages.

Honestly, I don't know how to interpret your comments. Can we discuss this offline?

@josepjaume
Copy link
Copy Markdown
Contributor

This is more of a feature definition issue than a development one. That's why I didn't want to seem like I was criticizing your work. It's just that I'm worried that we've been too ambitious with this feature and is introducing a level of complexity we can later on regret.

I originally suggested having a bit of code duplication for this feature, but duplicating it as a whole feels like too much. Was really all the functionality needed?

Anyway, I don't want to get in the middle of this. If @decidim/product feels like this is the right way to go, let's merge it.

IMHO we should be less ambitious on the flexibility part and be more opinionated instead. In this scenario, a predefined survey structure would resulted in a cleaner codebase, and also less cognitive overhead to the end user. It's also a good opportunity to unify criteria regarding how to use the platform.

@josepjaume
Copy link
Copy Markdown
Contributor

So to summarize it: This is a whole lot of code that will need to be maintained, and will generate a lot of overhead. Also, since its vast majority is JavaScript and we don't have an appropriate test coverage for that, will probably be very brittle. Is it really worth it?

@josepjaume
Copy link
Copy Markdown
Contributor

By the way, I don't want to be confrontational about it, but here's what we actually said before when discussing the issue:

I rather create these two specific features at the meetings component than trying to reuse the surveys component inside another one.

I think duplicating part of the code would be more maintainable and easier to reason about, also less error-prone. It also forces us to think in the specific use case instead of invoking the “more flexibility is always good” mantra.

Which is actually what I'm advocating right now.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Jun 4, 2018

So to summarize it: This is a whole lot of code that will need to be maintained, and will generate a lot of overhead. Also, since its vast majority is JavaScript and we don't have an appropriate test coverage for that, will probably be very brittle. Is it really worth it?

That's what I meant with my comment on #3331 (comment), so 👍

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Also, since its vast majority is JavaScript and we don't have an appropriate test coverage for that, will probably be very brittle.

Just a note about this. Surveys JS code is quite thoroughly covered. It's done through system specs, though, so unless those are copied and adapted too (not sure if this has happened or not), the new JS will indeed be uncovered.

@andreslucena
Copy link
Copy Markdown
Member

Sorry for the delay on entering on this issue, I don't understand the problem, maybe we could have a video conference, but as far as I see the proposed/discussed solution is the one that was implemented: to copy paste the survey module inside meetings so we don't add extra complexity.
I think we need all the features of survey module, specially the dynamic questionnary logic and exporting to CSV the answers.

There's an example form of what we're trying to reproduce here. There was a process at least that they've already used surveys to inscriptions on meetings (it's offline the form right now).

Also, since its vast majority is JavaScript and we don't have an appropriate test coverage for that, will probably be very brittle.

This is another issue (we don't have an appropriate test coverage for JavaScript code)

@josepjaume
Copy link
Copy Markdown
Contributor

josepjaume commented Jun 4, 2018

Well, as @deivid-rodriguez pointed out, it is covered, via system specs, but it's not in this PR. We definitely should at least add the same tests here - copying the code isn't enough. Also I don't think we ever suggested copy-pasting the whole code - we said that having a bit of duplication would make sense, but of course copying the whole of it raises some flags.

In any case, there's a lot of middle ground in between - you should be able to share part of the code via a javascript library without resorting to something like component nesting at a higher level.

I don't think taking this road is wise, from the product point of view.

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Jun 4, 2018

Well, as @deivid-rodriguez pointed out, it is covered, via system specs, but it's not in this PR.

@josepjaume I believe that I have ported all existing specs from the surveys component. If you tell me what is missing I will add it. The only "missing" thing is the export functionality (code and specs).

On the other hand, this will be difficult to clarify here, let's wait for @decidim/product and do an offline meeting if you all agree.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

deivid-rodriguez commented Jun 4, 2018

Personally, I feel I should apologize for giving my opinion without being aware of the full specs of the feature. Had I known the whole of surveys needed to be replicated, I would have probably not recommended copying the code but giving it a little more thought instead.

I still feel that surveys are not a good fit for a component and feel like a different entity. Otherwise, why does the framework restrict a single survey record per survey component?

We could explore moving "surveys" to core + admin as "forms" (or something as descriptive but less ambiguous), and adding a configuration on both participatory space and component manifests to signal that they can have an attached form. That would add links in the backend to create and attach the form to the component/space, and display it in the frontend somehow.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

By the way, @rbngzlv is right, the new system specs live in decidim-meetings/spec/shared/manage_questionnaires_examples.rb.

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Jun 4, 2018

We could explore moving "surveys" to core + admin as "forms" (or something as descriptive but less ambiguous), and adding a configuration on both participatory space and component manifests to signal that they can have an attached form. That would add links in the backend to create and attach the form to the component/space, and display it in the frontend somehow.

Maybe my approach wasn't the most correct, but this was my goal from the beginning, make the surveys code "reusable/shared".

@xabier
Copy link
Copy Markdown

xabier commented Jun 4, 2018

hi, sorry to join now, maybe it is too late to stress how convenient was the idea to make components be reusable within components. we need to deliver this now. so a proper implementation might have to wait for a latter refactoring. from the product side it is clear that we need a full features Survey component to be operative inside a Meeting, in different scenarios:

  • Complex registration
  • Evaluation (with open questions, because there are far too many types of Meetings to pre-define a fixed evaluation survey)
  • Additional uses: surveys during the meeting or previous to the meeting (but different from registration)

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Maybe my approach wasn't the most correct, but this was my goal from the beginning, make the surveys code "reusable/shared".

Yeah, I'm fully aware that this was your initial intention and that I explicitly discouraged you from the idea by recommending to duplicate code instead. That's why I apologize, because I'm not sure it was the right advice after seeing the final diff.

@josepjaume
Copy link
Copy Markdown
Contributor

josepjaume commented Jun 5, 2018

I've been thinking about it. First of all, I also have to apologize as I probably didn't understand the scope of the issue enough - I didn't know what you wanted is an exact replica of the surveys component. Also, I hadn't seen the system specs as GitHub collapses them (the file is too long) so we're covered there.

In any case, I don't think using components within components would have worked, as the abstraction wouldn't resist it - eventually we would have found a block in the way.

As @deivid-rodriguez says, it absolutely looks like surveys could live as a separate entity, but even in that scenario, rails' architecture would make it very difficult to reuse. It feels like we're pushing the boundaries of code reusability within the MVC model. Sharing views, controllers, models across entirely separate functionalities has already been proven really difficult with components - I can't imagine adding other entities to the list.

IMHO, the assumption that this code reuse was feasible lead us to think adding arbitrary surveys to meetings would be easy - it is most certainly not.

We're now in a situation where we're adding a lot of complexity that will end up in maintenance headaches & functionality divergence (as surveys will probably keep improving in parallel) for little value.

I believe we should have found a compromise in the definition of the functionality instead of sacrificing code quality & maintenance, as the price for this overhead will be certainly paid in the future.

@josepjaume
Copy link
Copy Markdown
Contributor

So in the end, what I'm saying is: Do we really need something like this - a full surveys functionality within meetings? If we do, copy-pasting the whole surveys feature is certainly not the best way to do that - we should invest some time in thinking about it as a whole and do it in a sustainable way. And that needs time - which we don't have.

IMO, resorting to the "can be refactored later" solution is dangerous, especially with such amount of code. I would seriously rethink whether we need this right now or not.

Again @rbngzlv I'm sorry to get you trapped in this and I appreciate your work. It's just really tricky to get right. Hope you don't take it personally!

@andreslucena
Copy link
Copy Markdown
Member

So in the end, what I'm saying is: Do we really need something like this - a full surveys functionality within meetings?

Please, the discussion it's not if we need this, it's how we implement this. Maybe we could re-define it, but I think it's pretty clear that we need it because people ask it (on MetaDecidim meetings, also other public administrations and organizations) and by how people used the software before. For instance, on Decidim Barcelona itself this was already done, 4 times on the same process, using the survey functionality itself (link only accessible by admins).

imagen

imagen

As I said before, for me the better would be to have a fast online meeting between the three of us (@decidim/product @decidim/lot-core @decidim/lot-mods ) so we can have a consensus on how to work this out.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Jun 7, 2018

@andreslucena maybe asking the design team could help too? We could try exploring other workflows, for example when you join a meeting you are sent a notification/email with a link to a private survey that you can fill in.

I don't see any specification for this issue: workflows, mockups, etc. Should the user answer this after they join the meeting? Or right before? Is answering compulsory to join the meeting? I only see this in #3044:

Ask for specific services for meetings_: care-taking e.g. space. as a checkbox form that is defined at the admin panel

@xabier
Copy link
Copy Markdown

xabier commented Jun 7, 2018

@josepjaume 👍

So in the end, what I'm saying is: Do we really need something like this - a full surveys functionality within meetings?

Absolutely yes. This has been discussed at length in several meetings. It is a repeated demand and a clear decision already made by @decidim/product

@josepjaume
Copy link
Copy Markdown
Contributor

josepjaume commented Jun 7, 2018

If having the exact set of features of surveys available in meetings is a must, then we should definitely find another way around this. Duplicating the entire codebase doesn't look like a suitable solution, but neither is having sub-components the way it was initially described. There's more subtle ways to reuse functionality within features like creating a survey abstraction the same way we did in comments, so it can be reused in other places.

This would certainly imply a lot of work, but I don't think there's any other way around that. From the code quality perspective, the current approach doesn't meet the criteria. I get it's a popular demand, but we shouldn't rush it into the codebase if we don't feel it's ready.

I'm sorry because I know @rbngzlv put a lot of work into this, but it's honestly how I see it. I don't think we correctly evaluated the risks / costs / workload involved in adding this functionality.

@carolromero
Copy link
Copy Markdown
Member

We've talked on @decidim/product and we've arrived to a new consensus after discussing and thinking about the future implications for the framework. We think that the best course of action (between feature and maintainability), is that we change the approach a little bit, please give us feedback.
There could be multiple solutions regarding this issue:

  1. That the meetings component depends on (and uses) the surveys component. This can lead to maintenance problems in the future.
  2. Copy and reuse the survey component in meetings: this is what this RP says right now. This means having all the code duplicated and complex to maintain (a bug in surveys has to be copied also for inside-of-meetings surveys).
  3. Refactor surveys, put them in core under another name ("Forms" or something similar) and make them consume surveys and meetings. This would take a lot of work and time.
  4. Limit the functionality to certain predefined fields (name/email/accept LOPD and little more)
  5. Define the survey as a non-public component, and in the meeting you can make a reference to this form, in the two moments that we have detected (pre-event registrations and post-event satisfaction questionnaire). This would be with a dropdown where you select which survey is related to that meeting.

The option that looks the most balanced to us is number 5, BUT we need to find a way to ensure that only people registered for a meeting can access the survey.

Please give us feedback on this approach, If we can apply this solution we'd create a new issue with the updated specs.

@josepjaume
Copy link
Copy Markdown
Contributor

Hi! Should we close this PR then?

@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Jun 12, 2018

Yes, I close this, don't worry.

TLDR; As we agreed offline, we will investigate new ways in the next meeting.

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.

8 participants