Skip to content

Add correct call for conference speaker#10061

Merged
ahukkanen merged 2 commits intodecidim:developfrom
i-need-another-coffee:fix/10027
Nov 29, 2022
Merged

Add correct call for conference speaker#10061
ahukkanen merged 2 commits intodecidim:developfrom
i-need-another-coffee:fix/10027

Conversation

@alecslupu
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu commented Nov 11, 2022

🎩 What? Why?

As per #10027 we can see the conference speaker is not being defined. I have looked into the issue and i have seen that edit action is referring to conference speaker as @item, and the the selected_meetings helper was checking @current_speaker variable.

📌 Related Issues

Link your PR to an issue

Testing

  1. Sign in as admin
  2. Go to admin dashboard, conference, speakers, create a new speaker
  3. Fill in the required fields with any participant type
  4. Select a related meeting
  5. Save the form
  6. Edit the same speaker
  7. See that related meetings as being highlighted

📷 Screenshots

Please add screenshots of the changes you're proposing
image

♥️ Thank you!

@alecslupu alecslupu added module: conferences type: fix PRs that implement a fix for a bug labels Nov 11, 2022
@alecslupu alecslupu self-assigned this Nov 11, 2022
@andreslucena
Copy link
Copy Markdown
Member

I've actually arrived to the same conclusion and had a PR local with the same fix 😄

I didn't have the chance to send it as I wanted to replicate the exception in specs before opening it. The problem is that my specs are passing, and I wanted to avoid using a system spec for this one as they're slow (but on a second thought, maybe this only can be replicated through system specs?)

First I tried through a controller spec:

decidim-conferences/spec/controllers/admin/conference_speakers_controller_spec.rb:

# frozen_string_literal: true

require "spec_helper"

module Decidim
  module Conferences
    module Admin
      describe ConferenceSpeakersController, type: :controller do
        routes { Decidim::Conferences::AdminEngine.routes }

        let(:organization) { create(:organization) }
        let(:current_user) { create(:user, :confirmed, :admin, organization:) }
        let(:conference) { create(:conference, :published, organization:) }
        let(:conference_speaker) { create(:conference_speaker, conference:) }

        before do
          request.env["decidim.current_organization"] = organization
          request.env["decidim.current_conference"] = conference
          sign_in current_user
        end

        describe "GET index" do
          it "renders the index view" do
            get :index, params: { conference_slug: conference.slug }

            expect(response).to have_http_status(:ok)
            expect(response).to render_template(:index)
          end
        end

        describe "GET edit" do
          it "renders the edit view" do
            get :edit, params: { conference_slug: conference.slug, id: conference_speaker.id }

            expect(response).to have_http_status(:ok)
            expect(response).to render_template(:edit)
          end
        end

        describe "PATCH update" do
          context "when it has the right params" do
            let(:conference_speaker_params) do
              {
                existing_user: "false",
                full_name: "John Doe",
                conference_meeting_ids: [],
                position_en: "President",
                position_ca: "President",
                position_es: "Presidente",
                affiliation_en: "ACME Corp",
                affiliation_ca: "ACME Corp",
                affiliation_es: "ACME Corp"
              }
            end

            it "updates the speaker" do
              patch :update, params: { conference_slug: conference.slug, id: conference_speaker.id, conference_speaker: conference_speaker_params }

              expect(response).to have_http_status(:redirect)
              expect(flash[:notice]).not_to be_empty
              expect(response).to redirect_to(conference_speakers_path(conference.slug))
              expect(conference_speaker.reload.full_name).to eq("John Doe")
            end
          end

          context "when it doesn't have the right params" do
            let(:conference_speaker_params) do
              {
                existing_user: "false",
                full_name: "",
                conference_meeting_ids: [],
                position_en: "President",
                position_ca: "President",
                position_es: "Presidente",
                affiliation_en: "ACME Corp",
                affiliation_ca: "ACME Corp",
                affiliation_es: "ACME Corp"
              }
            end

            it "renders the edit view" do
              patch :update, params: { conference_slug: conference.slug, id: conference_speaker.id, conference_speaker: conference_speaker_params }

              expect(response).to have_http_status(:ok)
              expect(flash[:alert]).not_to be_empty
              expect(response).to render_template(:edit)
            end
          end
        end
      end
    end
  end
end

But this is passing.

Then I realized that actually I'd need to make a spec for the view:

decidim-conferences/spec/views/decidim/conferences/admin/conference_speakers/edit.html.erb_spec.rb:

# frozen_string_literal: true

require "spec_helper"

module Decidim
  describe "decidim/conferences/admin/conference_speakers/edit" do
    let(:organization) { create(:organization) }
    let(:user) { create(:user, :confirmed, :admin, organization:) }
    let(:conference) { create(:conference, :published, organization:) }
    let(:conference_speaker) { create(:conference_speaker, conference:) }
    let(:context) do
      {
        current_organization: organization,
        current_user: user,
        current_participatory_space: conference
      }
    end
    let(:form) do
      Decidim::Conferences::Admin::ConferenceSpeakerForm.from_model(conference_speaker).with_context(context)
    end

    before do
      view.extend DecidimFormHelper
      controller.extend Decidim::FormFactory
      allow(view).to receive(:conference_speaker_path).and_return("/admin/conferences/a-conference/speakers/1/edit")
      # rubocop:disable RSpec/AnyInstance
      allow_any_instance_of(Decidim::FormBuilder).to receive(:autocomplete_select)
      # rubocop:enable RSpec/AnyInstance

      assign(:item, conference_speaker)
      assign(:form, form)
    end

    it "renders the form" do
      expect(render).to include(conference_speaker.full_name)
      expect(render).to include("user-fields")
    end

    context "when the conference speaker doesn't have a full name" do
      let(:conference_speaker) { create(:conference_speaker, user: nil, full_name: "", conference:) }

      it "renders the form" do
        expect(render).to include("user-fields")
      end
    end
  end
end

As far as I understand the last example should fail (before the changes on this PR of course), but actually it's passing. As I said, maybe this bug can only be replicated through system specs?

@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Nov 14, 2022

@andreslucena is not clear for me, if we still need to do something here, or not.

I coul not replicate the 500 error described by #10026

@andreslucena
Copy link
Copy Markdown
Member

@andreslucena is not clear for me, if we still need to do something here, or not.

You're right, I wasn't clear. I'd love to have an spec for this error but I don't see any other way of doing this that isn't a system spec, and I can live without that.

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.

LGTM, thanks for the fix

@ahukkanen
Copy link
Copy Markdown
Contributor

Fixes #10026

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! Fixes the original issue as well as #10026.

@ahukkanen ahukkanen merged commit 90d6a36 into decidim:develop Nov 29, 2022
@alecslupu alecslupu deleted the fix/10027 branch November 29, 2022 17:23
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: conferences type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Related meetings aren't saved in conferences speakers form 500 error when saving a non-participant conference speaker without a full name

3 participants