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

Update specs #32

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Update specs #32

wants to merge 4 commits into from

Conversation

Dirk82
Copy link
Contributor

@Dirk82 Dirk82 commented Mar 8, 2022

While digging deeper into this gem, I stumbled on the failing tests.
The root cause has nearly always been the missing user_id for a blog post, which is required to be existent since the belongs_to :user defined in the post model is not marked as optional: true (a change introduced with Rails 5 after a quick investigation).
It's not an issue for category_id, as this is included in the factory and covers the presence in most cases (with exception of the system tests).

From my point of view there are two options how this can be treated in general and I'm happy to put this here for discussion (with partially prepared changes).

1. Mark the user and category as optional

The provided atom view checks for the presence of a user and includes the user's name if one has been set. So it seems to be a valid use case and I agree that it makes sense under some circumstances to leave the user unset in some cases.

2. Let the user and category be mandatory

On the other hand it's rather usual (from my perspective) that a blog post has an author assigned in order to show related articles from this author. The same holds for categories, e.g. for better filtering. I think most writers / bloggers want their content to be found and seen.

What has been done so far

To make the specs work for me, I went the second route. At least in terms of adjusting the specs. Based on that a spina_user factory has been introduced (shamelessly stolen from the spina gem 👮‍♂️) and used for the spina_blog_post. Also the system test for creating a post has been adapted to the UI.

To emphasise the second route, it is thinkable to make the model specs a little bit more expressive.
Also adding NOT NULL constraints on the database level could be an option here:

  • Post: user_id, category_id, maybe also for title and content as they have a presence validation already in place
  • Category: as name's presence is also validated, a database constraint should not hurt

I was thinking about if this could be a breaking change, but as at least Spina 2.1.0 (which requires at least Rails 6.1) is required for this gem, there does not seem to be any peril here and most likely all users have been confronted with the requirements of choosing an user and a category for a blog post.
Which leaves still the option that someone has overwritten / extended the models themselves and mitigated the need to fill in these fields...🙈

I'm looking forward to hear your thoughts / insights and happy to discuss this topic. 🙂

- create category upfront
- let capybara choose category and user ID
- merge necessary user and category IDs into attributes hash
@simmerz
Copy link
Member

simmerz commented Apr 25, 2024

Sorry it's taken some time to get to this! 😓

I think longer term it would be good to have this configurable, but there are good reasons why someone wouldn't care to publish to know who the posting user is, and likewise that there may not be a need to categorise articles.

To that end, I think marking as optional and moving on would be a good move here.

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.

2 participants