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.

Sign up for GitHub

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

Jump to bottom

Add support to verify touch option #246

Merged
merged 4 commits into from
May 30, 2024
Merged

Add support to verify touch option #246

merged 4 commits into from
May 30, 2024

Conversation

sairamsrinivasan
Copy link
Collaborator

This PR adds support to verify touch options in rspec

@sairamsrinivasan sairamsrinivasan marked this pull request as ready for review May 11, 2024 23:35
@sairamsrinivasan sairamsrinivasan mentioned this pull request May 12, 2024
Merged
dblock
dblock requested changes May 12, 2024
View reviewed changes
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I think we want with_touch (assumes true), with_touch(true), with_touch(false). Add tests for each combination.

Questions:

  1. Does touch appear in with_options? Add tests/examples/README.
  2. Is touch always set/never set, meaning do we need to treat absence of touch: true/false differently?

README.md Outdated
@@ -256,6 +256,11 @@ RSpec.describe Article do
it { is_expected.to embed_many(:comments) }
end

# when the touch option is provided, then we can also verify that it is set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

match other descriptions' style

"can also verify that touch is set"

@sairamsrinivasan
Copy link
Collaborator Author

sairamsrinivasan commented May 12, 2024 •
edited
Loading

I think we want with_touch (assumes true), with_touch(true), with_touch(false). Add tests for each combination.

Questions:

  1. Does touch appear in with_options? Add tests/examples/README.
  2. Is touch always set/never set, meaning do we need to treat absence of touch: true/false differently?

Great suggestion. Let me incorporate the feedback.

Does touch appear in with_options? Add tests/examples/README. -> I checked this by updating one of the embedded_in statements to be nested under with_options touch: true. The with_options matcher didn't respect this change.

Is touch always set/never set, meaning do we need to treat absence of touch: true/false differently? ->

I think it ultimately depends on the association. Based on mongoid's documentation for embedded_in associations, the default value for touch is true as of mongoid 9. Also, the touch option can take a value other than true/false in the form of a symbol. This symbol refers to a field defined in the parent association.

@sairamsrinivasan sairamsrinivasan marked this pull request as draft May 24, 2024 02:10
@sairamsrinivasan sairamsrinivasan force-pushed the add-support-to-verify-touch-option branch from 10894bf to 264e1c6 Compare May 27, 2024 21:59
@sairamsrinivasan sairamsrinivasan marked this pull request as ready for review May 27, 2024 22:04
@sairamsrinivasan
Copy link
Collaborator Author

sairamsrinivasan commented May 27, 2024 •
edited
Loading

@dblock - While implementing the suggested changes, I stumbled upon a few things. Specifying the touch option on a belongs_to or embedded_in association requires the parent document's class to be loaded before the child. If this doesn't happen, the relatable module cannot resolve the parent class. There's a NameError that gets swallowed by library code here: https://github.com/mongodb/mongoid/blob/master/lib/mongoid/association/relatable.rb#L461

I worked around this by requiring the parent class in the child that defines the belongs_to or embedded_in association since this statement doesn't take this situation into account: https://github.com/mongoid/mongoid-rspec/blob/master/spec/spec_helper.rb#L20

Thoughts on this?

I also pushed an updated changeset for review.

@sairamsrinivasan
Add support to match against touch
ef72704 
add support with with_touch matcher

add support for touch matcher

Update readme

revert section of readme

push tests

update changelog

require classes as the touch options requires that parent classes are loaded before the child

update documentation

generate appraisal files

fix rubocop

remove version on debug dev dependency

remove debug as a dev dependency

update readme

remove require debug

remove commented out code

update version

remove test on comment
@sairamsrinivasan sairamsrinivasan force-pushed the add-support-to-verify-touch-option branch from c472821 to ef72704 Compare May 27, 2024 22:45
dblock
dblock requested changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks nice and clean! Fix up the version change and it's good to go.

CHANGELOG.md Outdated Show resolved Hide resolved
lib/mongoid/rspec/version.rb Outdated Show resolved Hide resolved
@dblock dblock merged commit 9b74fc1 into mongoid:master May 30, 2024
11 checks passed
@dblock dblock mentioned this pull request May 30, 2024
Closed
@dblock
Copy link
Collaborator

dblock commented May 30, 2024 •
edited
Loading

@sairamsrinivasan Thank you for this and #245. Any interest in helping with this gem? (see #237) Maybe making the next release could be a start? If yes, email me dblock at dblock dot org with your rubygems login

@sairamsrinivasan
Copy link
Collaborator Author

@dblock Thanks for reviewing and merging this PR. I’d be more than happy to help with this gem. I’ll follow up through email when I get the chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sairamsrinivasan @dblock