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

Slugs only in the urls #82

Open
sbonelo-nje opened this issue Jan 10, 2025 · 5 comments
Open

Slugs only in the urls #82

sbonelo-nje opened this issue Jan 10, 2025 · 5 comments
Labels
Enhancement Feature enhancement Idea A feature or behavior idea

Comments

@sbonelo-nje
Copy link

Is your feature request related to a problem? Please describe.

Browsing the internet looking what's the recommended way to structure urls, especially for a blog - almost all (I didn't find any that suggests otherwise) suggest not to use the current url structure of Lotus.

A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Installing the app, and using the Article view for instance to fetch using the slug_url_kwargs in a subclass returns a 404 in tests.

Describe the solution you'd like
A clear and concise description of what you want to happen.

  • A possible way to configure whether to use the default url structure or a custom (slug is the only that makes sense honestly but hey) url structure, by a developer in each project.
  • Possible switch completely from the current to use url structure by default (I'm not so sure about this one though)

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

  • Overriding the get_object method to not use the other kwargs when filtering still raises the same error.

  • Browsing the view code, the mixins, down to the model itself, querysets and managers- I can't find the exact code that handles this filter. I get what the other filters are doing - like filtering for the language etc.

  • Debugging line by line of the test and indeed my article is created with the published status, the required fields and everything else is handled by the defaults. Am I missing something?

Additional context
Add any other context or screenshots about the feature request here.

@sveetch sveetch added Question Further information is requested Enhancement Feature enhancement Idea A feature or behavior idea and removed Question Further information is requested labels Jan 10, 2025
@sveetch
Copy link
Member

sveetch commented Jan 10, 2025

Hi,

This is something i thought because i know there is other formats for article detail URL but there is actually no way to drop the date from it.

This is because actually as you can see in model the slug is only enforced to be unique with the publish date, it means you can use the same slug for different publish date. This may be required sometime from some SEO strategy.

Then so in code, the publish date in URL is used to perform queryset.

You could imagine to override the view to remove the date usage from queryset and keep only the slug but it would create a flaw because slug is not unique globally, only per publish date so someone could write differents articles with the same slug on different date.

I've started to think something more flexible that would allow to define a different strategy (slug only, slug with date, just the id, etc..) but it means some work since the date+slug pattern is used in model (to build the absolute url), views, urls and tests, also there is currently a database uniqueness constraint for this in the Article table.

So finally, i won't reject your request but i will need some time to find the proper way to do this and then to implement it. Don't expect it for now.

@sbonelo-nje
Copy link
Author

sbonelo-nje commented Jan 10, 2025

I hear you, and I'm willing to help get this done - under your guidance of course.

I think, the most critical of all of these is the db constraint on the model. All the other places can be easily reconfigured in subclasses if a developer wishes to change the default setup.

  • Does the article model need to be swappable? And then define a # (get_article_model) util to get the installed model instead of directly importing it?
    I'm not really familiar with how the swappable api is implemented or works in Django, it's not documented on the Django Docs but it is used in the User model and so far it works. I've dug down on the auth app and nothing magical is - happening besides returning a settings attribute (AUTH_USER_MODEL).

Another util to supplement this, that get's the LOTUS_ARTICLE_CONSTRAINT_FIELDS att from the settings object?

These are just my ideas, I'm not that experienced with Django, I just have the understanding of how it works.

Everything else, like I said - can just be overriden by a Dev.

@sbonelo-nje
Copy link
Author

We would need to have like an abstract model for the Article as well, with everything on the current model (and maybe be where the default model constraints can be added? That way anyone can easily choose to add them or remove them in their subclass

@sveetch
Copy link
Member

sveetch commented Jan 10, 2025

Yeah there is an issue (#7) about modularity since Lotus was inspired from Zinnia that allowed to define custom model for Article and yes it worked like the swappable user model. It is a relevant solution however with the problem of the slug contraint with publish date you would need to override more than models, it would probably involves urls and views at least.

So for now i need to find what would be more useful, either a feature to define rule about Article slug (where the db uniqueness constraint would be removed in profit of a soft constraint from code) or the modularity that if done right would allow more behaviors but less easy.

@sbonelo-nje
Copy link
Author

sbonelo-nje commented Jan 10, 2025

In favor of removing the db constraint, I am merely just thinking for now.

What if we :

  1. Define a new settings constant LOTUS_ARTICLE_URL_USE_PUBLISH_DATE, which should be True by default for backwards compatibility.

  2. Define a util 'get_article_use_publish' that returns the Above constance from the settings.

  3. Define a util 'get_article_url_path_str' that uses the above util, if the returned is True : return the current article url structure, else returns a new structure that uses only the slug 'slug:slug'. The returned data must be a string by all times as this is what Django expects.

Then import this util in the urls module, replace the current path with the util - called.

  1. Update the ArticleDetailView.get_object, specifically the 'q_kwargs' dict. By default set it the slug kwarg only, then implement an if statement that uses the util defined in number 2, updating the 'q_kwargs' dict with the extra kwargs (thie would be the default since the setting will be True by default).
    Then the rest of the code will continue as normal. #NB Import the the util defined in number 2.

  2. In the ArticleModel.build_absolute_url :

Define a q_kwargs' dict. By default set it the slug kwarg only, then implement an if statement that uses the util defined in number 2, updating the 'q_kwargs' dict with the extra kwargs (thie would be the default since the setting will be True by default). #NB Import the the util defined in number 2.

Pass the dict to the reverse kwargs.

  1. Lastly, remove the 'publish_date' db constraint on the Article model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Feature enhancement Idea A feature or behavior idea
Projects
None yet
Development

No branches or pull requests

2 participants