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

[#48114] Create Meetings outside of a Project #12783

Merged

Conversation

aaron-contreras
Copy link
Contributor

@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from f192fd8 to a1dbbaf Compare June 5, 2023 23:18
@aaron-contreras aaron-contreras self-assigned this Jun 6, 2023
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from d0df4fe to 74bd38d Compare June 7, 2023 15:29
@aaron-contreras aaron-contreras marked this pull request as ready for review June 7, 2023 15:47
@aaron-contreras aaron-contreras requested a review from a team June 7, 2023 15:49
@aaron-contreras aaron-contreras removed their assignment Jun 7, 2023
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from 8d5382f to 20a883f Compare June 7, 2023 18:17
@aaron-contreras aaron-contreras removed the request for review from a team June 8, 2023 20:49
@aaron-contreras aaron-contreras marked this pull request as draft June 8, 2023 20:49
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from f158c9c to 49e0ca8 Compare June 8, 2023 22:19
@aaron-contreras aaron-contreras marked this pull request as ready for review June 8, 2023 22:20
@aaron-contreras aaron-contreras requested a review from a team June 8, 2023 22:20
@dombesz dombesz requested review from dombesz and removed request for a team June 12, 2023 09:10
Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

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

  1. I'm not entirely sure if the two controllers are required, to simplify the code we could create a meetings_scope like:
def meetings
  @projects = Project.allowed_to(User.current, :view_meetings)
  @projects = @projects.where(project_id:) if project_id   
  @meetings = Meeting.where(project: projects).from_today
end

This could be used in both of the index pages, without further ado.
The scope can be used to build the Meeting in the create action and it will automatically assign the project_id:

def create
  @meeting = @meetings.build(meeting_params)
end
image
  1. Submitting the meeting form without selecting a project results in an error page.

Jun-15-2023 13-06-44

@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from 5c26e2f to 3116e45 Compare June 16, 2023 14:14
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch from 3116e45 to ce7da24 Compare June 16, 2023 14:34
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch from 1a4b5b3 to 70de795 Compare July 5, 2023 17:08
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from 75606d8 to 8c1467e Compare July 7, 2023 15:04
@aaron-contreras aaron-contreras marked this pull request as draft July 7, 2023 15:04
@aaron-contreras
Copy link
Contributor Author

As discussed, I've moved the PR back to a draft state in order to modify the form implementation to use turbo-streams instead of turbo-frames.

Thanks for the catch Attila!
Adds a `turbo-frame` elemenet that dynamically fetches the participants section
upon a change to the selected project from the dropdown.
…eter`

**NOTE**: `typeToSearchText` was not an accepted option by `ng-select` according to its
docs. Removing it as it's dead code and the `placeholder` property was already provided
and this is where the "Type to search" placeholder was actually being fetched from.
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch 2 times, most recently from fd9693d to d971435 Compare July 7, 2023 16:30
As suggested by Jonas, this is much cleaner and extensible of an approach.
The `src` attribute hack of the prior `turbo-frame` approach is not really
of my liking for this anymore. As a side-benefit, this is quite a reusable
Stimulus action now.
@aaron-contreras aaron-contreras force-pushed the implementation/48114-create-meetings-outside-of-a-project branch from d971435 to 4309282 Compare July 7, 2023 16:52
@aaron-contreras aaron-contreras marked this pull request as ready for review July 10, 2023 10:30
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

The things I am complaining about are leftovers of the pervious attempt. Feel free to merge this PR yourself, @aaron-contreras after those have been removed since the overall change is good.

request.path == new_meeting_path
end

def options_for_project_selection
Copy link
Contributor

Choose a reason for hiding this comment

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

This can now be removed since the functionality is now provided by the filters passed into the autocompleter.

@@ -99,6 +101,8 @@ en:

project_module_meetings: "Meetings"

project_selection_placeholder: "Select project"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed since the autocompleter is implemented in Angular now.

rescue ActiveRecord::RecordNotFound
render_404
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Always good to get rid of those controller specific overrides if they are not needed. 🙇

@@ -40,7 +40,10 @@ class Engine < ::Rails::Engine
project_module :meetings do
permission :view_meetings, meetings: %i[index show], meeting_agendas: %i[history show diff],
meeting_minutes: %i[history show diff]
permission :create_meetings, { meetings: %i[new create copy] }, require: :member
permission :create_meetings,
{ meetings: %i[new participants_section create copy] },
Copy link
Contributor

Choose a reason for hiding this comment

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

With the change to turbo-stream, there no longer is a participants_section method added to the controller so this addition can be reverted.

@aaron-contreras aaron-contreras merged commit 7cdc930 into dev Jul 17, 2023
12 checks passed
@aaron-contreras aaron-contreras deleted the implementation/48114-create-meetings-outside-of-a-project branch July 17, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants