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

feat: create organizations slug #393

Closed
wants to merge 7 commits into from

Conversation

MarioRodrigues10
Copy link
Member

No description provided.

@MarioRodrigues10 MarioRodrigues10 self-assigned this Aug 27, 2023
@MarioRodrigues10 MarioRodrigues10 linked an issue Aug 27, 2023 that may be closed by this pull request
2 tasks
@MarioRodrigues10 MarioRodrigues10 added this to the 1.0.0 milestone Aug 27, 2023
@MarioRodrigues10 MarioRodrigues10 added enhancement New feature or improvements frontend Frontend related backend Backend related database Database related labels Aug 27, 2023
@reviewpad
Copy link

reviewpad bot commented Aug 27, 2023

AI-Generated Summary: This pull request involves many changes across the AtomicWeb Elixir application relating to the handling of organizations.

The primary change is a structural shift in the routing system. It moves from using organization IDs (organization_id) to using organization handles (handle) in the route paths. This change in identifiers has been implemented throughout numerous live view modules of various features such as Activities, Departments, Partnerships, Memberships, Boards, Home, Speakers, Announcements, and Organizations. Changes in these places resulted in modifications in the mount/3, handle_params/3 and apply_action/3 functions, and in the routing helper methods.

Other notable changes include:

  • The core Organization model now includes a new required field 'handle', with corresponding changes in associated tests, fixtures, and migrations.
  • Numerous modules now rely on new functions get_organization_by_handle/1, get_organization!/1 and change_organization_handle/2 which were added to the Organizations module to manage organizations by their handles.
  • Some updates in tests and seeds where the new 'handle' attribute has been added.
  • A few refactoring changes identified, including: removal of an unnecessary blank line, simplification of preloading data, removal of duplicate code, and improvements of code readability.

The overall aim appears to increase the readability of the URLs, possibly by making identifiers within the URL more user-friendly, and to enforce unique handles for each organization to avoid routing issues.

These alterations would significantly affect the functionality and performance of the application, and thorough tests should be conducted to ensure the routing works as expected without negatively impacting the user experience or system performance.

@feliciofilipe feliciofilipe changed the title feat: create organizations handle feat: create organizations handler Aug 29, 2023
@ruilopesm ruilopesm removed this from the 1.0.0 milestone Aug 30, 2023
defp validate_slug(changeset) do
changeset
|> validate_required([:slug])
|> validate_format(:slug, ~r/^[a-zA-Z0-9_.]+$/,
Copy link
Member

Choose a reason for hiding this comment

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

Is it a good idea to allow case sensitive slugs?

Copy link
Member

Choose a reason for hiding this comment

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

I agree they should be case insensitive. Meaning "cesium" and "CeSIUM" both stand for the same slug.

lib/atomic/activities.ex Outdated Show resolved Hide resolved

## Examples
iex> get_organization_by_slug("foo_bar")
%Organization{}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
%Organization{}
%Organization{}

@@ -213,7 +238,7 @@ defmodule Atomic.Organizations do
nil

"""
def get_role(user_id, organization_id) do
def get_role(user_id, organization_id) when is_binary(user_id) and is_binary(organization_id) do
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

@@ -10,7 +10,7 @@
<%= if not @empty and @has_permissions do %>
<div class="hidden lg:border-orange-500 lg:block">
<%= live_patch("+ New Activity",
to: Routes.activity_new_path(@socket, :new, @current_organization),
to: Routes.activity_new_path(@socket, :new, @current_organization.slug),
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, interpolated ~p values are encoded via the Phoenix.Param protocol. For example, a %Post{} struct in your application may derive the Phoenix.Param protocol to generate slug-based paths rather than ID based ones. This allows you to use ~p"/posts/#{post}" rather than ~p"/posts/#{post.slug}" throughout your application. See the Phoenix.Param documentation for more details.

What about implementing this? Then we would only need to write @current_organization instead of @current_organization.slug 💡

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to implement this 🙏🏼

Comment on lines +18 to +19
|> assign(:id, id)
|> assign(:slug, slug)}
Copy link
Member

Choose a reason for hiding this comment

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

Why are these assigns needed?

lib/atomic_web/live/membership_live/index.html.heex Outdated Show resolved Hide resolved
@@ -13,5 +14,6 @@ defmodule Atomic.Repo.Migrations.CreateOrganizations do
end

create unique_index(:organizations, [:name])
Copy link
Member

Choose a reason for hiding this comment

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

We don't need an index by name. I know it was not added now, but, please, remove it 🙏🏼

@ruilopesm ruilopesm changed the title feat: create organizations handler feat: create organizations slug Sep 1, 2023
Comment on lines +155 to +162
def get_activity_organizations!(activity, preloads \\ [:departments]) do
Repo.preload(activity, preloads)
|> Map.get(:departments, [])
|> Enum.map(& &1.organization_id)
|> Enum.map(fn id ->
Organizations.get_organization!(id).slug
end)
end
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this anymore, since an activity is, for now, associated to only one organization.

Comment on lines +102 to +128
%{
key: :departments,
title: "Departments",
icon: :cube,
url: Routes.department_index_path(conn, :index, current_organization.slug),
tabs: []
},
%{
key: :partners,
title: "Partners",
icon: :user_group,
url: Routes.partner_index_path(conn, :index, current_organization.slug),
tabs: []
},
%{
key: :memberships,
title: "Memberships",
icon: :user_add,
url: Routes.membership_index_path(conn, :index, current_organization.slug),
tabs: []
},
%{
key: :board,
title: "Board",
icon: :users,
url: Routes.board_index_path(conn, :index, current_organization),
},
Copy link
Member

Choose a reason for hiding this comment

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

These are admin pages. Why are you adding them here?

@@ -10,7 +10,7 @@
<%= if not @empty and @has_permissions do %>
<div class="hidden lg:border-orange-500 lg:block">
<%= live_patch("+ New Activity",
to: Routes.activity_new_path(@socket, :new, @current_organization),
to: Routes.activity_new_path(@socket, :new, @current_organization.slug),
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to implement this 🙏🏼

@diogogmatos
Copy link
Member

1 similar comment
@diogogmatos
Copy link
Member

@joaodiaslobo
Copy link
Member

Will be done in the future

@ruilopesm ruilopesm deleted the mr/create-organization-handle branch August 6, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related database Database related enhancement New feature or improvements frontend Frontend related
Projects
Development

Successfully merging this pull request may close these issues.

Create organizations handle
6 participants