Skip to content

Fix missing fields on duplicate meetings functionality#9899

Merged
ahukkanen merged 8 commits intodecidim:developfrom
i-need-another-coffee:fix-9848
Nov 29, 2022
Merged

Fix missing fields on duplicate meetings functionality#9899
ahukkanen merged 8 commits intodecidim:developfrom
i-need-another-coffee:fix-9848

Conversation

@alecslupu
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu commented Oct 17, 2022

🎩 What? Why?

Fix the copy meeting functionality in 0.28.

📌 Related Issues

Testing

  1. Go to 'Meetings components (admin)'
  2. Click on 'Duplicate meeting'
  3. Change the date and some other information, check that meeting type is set to hybrid, scroll down to click Copy button
  4. The meeting is created as In person type

♥️ Thank you!

@andreslucena
Copy link
Copy Markdown
Member

I changed the PR description a bit so its easier to review.

@andreslucena andreslucena added module: meetings type: fix PRs that implement a fix for a bug labels Oct 17, 2022
@andreslucena
Copy link
Copy Markdown
Member

This is solving the original report, but as far as I see, there are others fields that aren't in the MeetingCopyForm that at least I expected to see there. (Sorry that I didn't add it to the original bug report!!)

This is the "Duplicate meeting" form:

image

And this is the original Edit form with all the differences highlighted:

image

As a summary:

  • The required validation for type
  • The help texts
  • The attributes: :iframe_embed_type, :comments_enabled, :comments_start_time, :comments_end_time, :iframe_access_level, :registration_type, :registration_url, :category

As this is getting bigger than expected initially, I see two options:

  1. If you have the time/availability, for you to fix that in these new details in this same PR
  2. If not, we can accept it as is (technically, its fixing the original report). Then I could tackle these changes and fix it in another PR.

So, whatever you prefer @alecslupu, feel free to say no if you don't have the time.

@alecslupu
Copy link
Copy Markdown
Contributor Author

@andreslucena , the following extra fields have been added :


            iframe_embed_type: meeting.iframe_embed_type,
            iframe_access_level: meeting.iframe_access_level,
            comments_enabled: meeting.comments_enabled,
            comments_start_time: meeting.comments_start_time,
            comments_end_time: meeting.comments_end_time,
            registration_type: meeting.registration_type,
            registration_url: meeting.registration_url,
            attending_organizations: meeting.attending_organizations,
            reserved_slots: meeting.reserved_slots,
            customize_registration_email: meeting.customize_registration_email,
            registration_form_enabled: meeting.registration_form_enabled,
            registration_email_custom_content: meeting.registration_email_custom_content

There may be some others like audio_url, video_url

@andreslucena
Copy link
Copy Markdown
Member

You're adding them to the Command, but as far as I understand they also need to be added in the Form (https://github.com/decidim/decidim/blob/develop/decidim-meetings/app/commands/decidim/meetings/admin/copy_meeting.rb). There should also be the required validation.

The help texts (arrows in my last screenshot) are still missing. They need to be added at https://github.com/decidim/decidim/tree/develop/decidim-meetings/app/views/decidim/meetings/admin/meeting_copies/_form.html.erb

We would also need specs for both classes (Form and Command).

Thanks!

@alecslupu alecslupu self-assigned this Nov 9, 2022
@alecslupu alecslupu changed the title Fix: Error duplicating hybrid meetings Refactor duplicate meetings functionality. Nov 10, 2022
@alecslupu alecslupu changed the title Refactor duplicate meetings functionality. Refactor duplicate meetings functionality Nov 10, 2022
@alecslupu alecslupu changed the title Refactor duplicate meetings functionality Improve duplicate meetings functionality Nov 10, 2022
@alecslupu alecslupu requested review from ahukkanen, andreslucena and mrcasals and removed request for mrcasals November 10, 2022 22:12
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I really like this approach, as we'll tackle the problem from the root: we just forgot that this feature exist and when we change something in the new form we don't update that in the copy form.

By removing this duplication we'll just fix this problem now and on the future too. Awesome! 🚀

I have a couple suggestions.

Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Two more small changes and we're good to go!

Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽

@andreslucena andreslucena changed the title Improve duplicate meetings functionality Fix missing fields on duplicate meetings functionality Nov 14, 2022
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Nice! All fields copied as expected.

@ahukkanen ahukkanen merged commit 43b7e94 into decidim:develop Nov 29, 2022
@alecslupu alecslupu deleted the fix-9848 branch November 29, 2022 17:24
entantoencuanto added a commit that referenced this pull request Dec 16, 2022
* develop:
  Redesign: action buttons (#9852)
  Integrate reported users in Global moderation (#10018)
  Fix resource_icon with component or manifest nil (#10134)
  Revent Docker actions to Ubuntu 20.04 due to OpenSSL issues (#10142)
  Fix push notifications URL method (#10017)
  Fix typo in README (#10110)
  Add correct call for conference speaker (#10061)
  Fix missing fields on duplicate meetings functionality (#9899)
  Fix translations missing on admin log (#9889)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: meetings type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error duplicating hybrid meetings

3 participants