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

Allow setting first-time-slug #283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tuxified
Copy link

@Tuxified Tuxified commented Jul 4, 2024

Hi,

Thanks for this package, it has been helpful for us. For our use case we found one issue, this PR is meant as a starting point for discussion.

In our application we allow users to set the slug. This works fine for content that already exists (is persisted to DB), but when creating new content the slug generator will override the given slug resulting in duplicates.

If you want to, I can also add tests to demonstrate what it fixes?

Best,
Tonći ( @Tuxified )

In our application we allow users to set the slug.
This works fine for content that already exists (is persisted to DB), but when creating new content the slug generator will override the given slug resulting in duplicates.
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

If you want to, I can also add tests to demonstrate what it fixes?

Yes, please

@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 526a9920e83446ab385ed7c91fd6944c1cd21449-PR-283

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-2.3%) to 93.182%

Files with Coverage Reduction New Missed Lines %
lib/ecto_autoslug_field/slug_generator.ex 1 96.55%
Totals Coverage Status
Change from base Build 1d19dd7a46c87f2890ebe7829e49458484e7bfe1: -2.3%
Covered Lines: 41
Relevant Lines: 44

💛 - Coveralls

@Tuxified
Copy link
Author

Tuxified commented Jul 8, 2024

Hi @sobolevn,

I've run into a little trouble with the current test setup. I feel the test cases are not cleanly separated at the moment.
I hope I can clarify what I mean:

In the setup block of (for example of SlugGeneratorTest) we setup 1 fixture:

setup do
  {:ok,
    %{
      user: User.changeset(%User{}, @valid_attrs),
      opts: [
        to: :simple_slug,
        slug_builder: &build_slug/2
      ]
    }}
end

and then we have a unit test to test 1 function (SlugGenerator.maybe_generate_slug/3)

test "maybe_generate_slug with multiple sources", fixture do
  changeset =
    maybe_generate_slug(fixture.user, [:name, :company], fixture.opts)

  assert changeset.changes.simple_slug == "nikita-sobolev-wemake-services"
end

However, the User.changeset/2 function in our setup block calls the function under test (SlugGenerator.maybe_generate_slug/3) several times already, tainting our test case. In my opinion the unit test doesn't describe the correct behaviour as the slugs are already generated by the time our unit test runs.

WDYT ? If you agree with me, is it ok if I propose a change in a different PR?

@sobolevn
Copy link
Member

sobolevn commented Jul 8, 2024

I have a very little memory of this code base (I wrote it 8 years ago), so, please feel free to improve it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants