Skip to content

Add action_text_attachment helper to FixtureSet #40289

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

Merged

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Sep 25, 2020

Summary

From within an action_text/rich_texts.yml file, generate an
<action-text-attachment sgid="..."></action-text-attachment> element
with a valid sgid attribute that references another FixtureSet record.

hello_world_review_content:
  record: hello_world (Review)
  name: content
  body: <p><%= ActionText::FixtureSet.attachment("messages", :hello_world) %> is great!</p>

@rails-bot rails-bot bot added the actiontext label Sep 25, 2020
ActiveSupport.on_load(:active_support_test_case) do
require "active_record/fixtures"

ActiveRecord::FixtureSet.context_class.include ActionText::FixtureFileHelpers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will likely need some guidance.

I tried to call FixtureSet.context_class... without the ActiveRecord scope, but had autoloading issues. Is there a better way?

@@ -0,0 +1,12 @@
module ActionText
module FixtureFileHelpers
def action_text_attachment(fixture_set_name, label)
Copy link
Contributor Author

@seanpdoyle seanpdoyle Sep 25, 2020

Choose a reason for hiding this comment

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

Related to https://github.com/rails/rails/pull/40289/files#r495132398, I had considered making this its own ActiveRecord::FixtureSet clone, and forcing callers to fully qualify their calls as if they were calling something like identify:

body: "<p><%= ActionText::FixtureSet.action_text_attachment %></p>"

That would avoid the auto-loading issues, but could be verbose. If this method is prefered, we could drop the action_text prefix and instead declare ActionText::FixtureSet.attachment, which isn't half-bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After more consideration, I've pushed up 7dfd977 to propose those changes instead.

@seanpdoyle seanpdoyle force-pushed the action-text-attachment-fixtures branch 2 times, most recently from 40d84c7 to 7dfd977 Compare September 25, 2020 17:39
module ActionText
class FixtureSet
def self.attachment(fixture_set_name, label, column_type: :integer)
signed_global_id = ActiveRecord::FixtureSet.signed_global_id(fixture_set_name, label, column_type: column_type, for: ActionText::Attachable::LOCATOR_NAME)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is a step too far and 7dfd977 is better, but I've pushed up the signed_global_id part to ActiveRecord::FixtureSet.

The tests for ActionText still pass, though I wasn't sure on where or how to add an ActiveRecord-side test for the new method.

@seanpdoyle seanpdoyle force-pushed the action-text-attachment-fixtures branch 3 times, most recently from 1ef8c24 to 65ddb55 Compare September 29, 2020 21:19
module ActionText
class FixtureSet
def self.attachment(fixture_set_name, label, column_type: :integer)
signed_global_id = ActiveRecord::FixtureSet.signed_global_id fixture_set_name, label,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@georgeclaghorn do you think that this is a reasonable addition to the ActiveRecord side of fixtures, or would we be better keeping it self-contained to the ActionText side?

hello_world_review_content:
record: hello_world (Review)
name: content
body: <p><%= ActionText::FixtureSet.attachment("messages", :hello_world) %> is great!</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@georgeclaghorn should this be declared as a fixture-local helper method, or is it reasonable to mirror the ActiveRecord::FixtureSet interface?

@seanpdoyle seanpdoyle force-pushed the action-text-attachment-fixtures branch from 65ddb55 to 85af078 Compare October 12, 2020 20:28
@georgeclaghorn
Copy link
Contributor

I have mixed feelings about the ActionText::FixtureSet.attachment API, but given the autoloading issues you ran into with action_text_attachment, I don’t think it’s worth revisiting. This is fine for now.

@@ -585,6 +585,14 @@ def identify(label, column_type = :integer)
end
end

def signed_global_id(fixture_set_name, label, column_type: :integer, **options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@georgeclaghorn do you feel like additions to both the ActiveRecord side and the introduction of the ActionText side are worthwhile?

Would this be better as an ActionText only change? An ActiveRecord only change that enables people to declare their own <action-text-attachment> elements manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see this extracted to GlobalID, but that can be done as a separate step. I don’t want to kick this to the next release just because of that.

From within an `action_text/rich_texts.yml` file, generate an
`<action-text-attachment sgid="..."></action-text-attachment>` element
with a valid `sgid` attribute that references another FixtureSet record.

```ruby
hello_world_review_content:
  record: hello_world (Review)
  name: content
  body: <p><%= action_text_attachment("messages", :hello_world) %> is great!</p>
```
@seanpdoyle seanpdoyle force-pushed the action-text-attachment-fixtures branch from 85af078 to 86d75a3 Compare October 30, 2020 17:00
@rails-bot rails-bot bot added the docs label Oct 30, 2020
@seanpdoyle seanpdoyle force-pushed the action-text-attachment-fixtures branch from 9f844d1 to 9fd59b2 Compare October 30, 2020 17:09
signed_global_id = ActiveRecord::FixtureSet.signed_global_id fixture_set_name, label,
column_type: column_type, for: ActionText::Attachable::LOCATOR_NAME

%(<action-text-attachment sgid="#{signed_global_id}"></action-text-attachment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this missing a trailing >?

Copy link
Contributor Author

@seanpdoyle seanpdoyle Oct 30, 2020

Choose a reason for hiding this comment

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

🙀 I wonder if that means the passing test isn't covering this behavior well enough?

I've tested this out in projects outside the Rails test suite, and it worked. Maybe the generated HTML is fixed by browser rendering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@georgeclaghorn I've closed the tag in e4e5f19.

@georgeclaghorn georgeclaghorn merged commit 76b33aa into rails:master Oct 30, 2020
@georgeclaghorn georgeclaghorn added this to the 6.1.0 milestone Oct 30, 2020
@seanpdoyle seanpdoyle deleted the action-text-attachment-fixtures branch October 30, 2020 19:56
@sedubois
Copy link
Contributor

sedubois commented Aug 6, 2022

@seanpdoyle I tried to attach a fixture with UUID as primary key:

    body: <div><%= ActionText::FixtureSet.attachment("authors", :john) %></div>

However the attachment is not found and renders as "☒". If I run the generated attachment's sgid through GlobalID::Locator.locate_signed(sgid, for: "attachable") I get the error ActiveRecord::RecordNotFound: Couldn't find Author with 'id'=513610136. Would this be a bug where it's looking for an integer ID instead of UUID?

Also while I'm here I note that forward slashes '/' appear necessary when referencing namespaced fixtures, e.g. ActiveStorage::Blob:

    body: <div><%= ActionText::FixtureSet.attachment("active_storage/blobs", :poster) %></div>

This contrasts with the way fixtures are loaded in test code where the slash is not necessary: blob = active_storage_blobs(:poster).

@sedubois
Copy link
Contributor

sedubois commented Aug 6, 2022

@seanpdoyle OK, I discovered the column_type parameter, this works (NB: it would be nice if it could be inferred automatically). Hopefully this provides extra documentation for anyone encountering a similar question. Thanks for providing this helper 🙂

    body: <div><%= ActionText::FixtureSet.attachment("authors", :john, column_type: :uuid) %></div>

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

Successfully merging this pull request may close these issues.

3 participants