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

add have_encrypt_attribute_matcher to test usage of the encrypts macro #1581

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

theforestvn88
Copy link
Contributor

@theforestvn88 theforestvn88 commented Dec 4, 2023

Since Rails 7.0, Active Record supports application-level encryption.
So i think it would be nice to have a matcher that help to test usage of the encrypts macro:

RSpec.describe Survey, type: :model do
    it { should have_encrypt_attribute(:access_code).downcase(true)  }
end

Copy link
Member

@matsales28 matsales28 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! I left some comments, please let me know if I can help you with anything to move this PR forward. There are also some linting issues. You can check them here.

Once again, thank you for contributing.

@theforestvn88 theforestvn88 force-pushed the add-encrypts-attributes branch 4 times, most recently from 12821b7 to 4e686dc Compare December 17, 2023 01:13
@danielolivaresd
Copy link

This would be a great addition. May I suggest a name change? have_encrypt_attribute have_encrypted_attribute

@matsales28
Copy link
Member

This would be a great addition. May I suggest a name change? have_encrypt_attribute have_encrypted_attribute

After a second look, this new method/matcher is quite similar to the serialize one, and our matcher is just serialize in that case. I'd say to keep a pattern across the matches to change to encrypts(:attr) instead of have_encrypt_attribute. I'd love to hear your thoughts on this @vsppedro.

@matsales28
Copy link
Member

@theforestvn88 I think it would be better to name this method as encrypts(:attr). Do you mind updating your PR with this change? Also, there's a small conflict on the README.md file, can you resolve it?

@vsppedro
Copy link
Collaborator

Sorry for taking so long to respond. I agree. I think your suggestion it's a good idea.

@theforestvn88
Copy link
Contributor Author

theforestvn88 commented Dec 25, 2023

At the first time i use shoulda, i surprised that it uses should have_many() meanwhile Rails use has_many() because i thought that it provide some thing like should <whatever rails do> to test <whatever rails do>. Turn out, you guys try to correct the grammar here.

Anyway, i just want to know that in this case what will we should use: should encrypts(:attr) or should encrypt(:attr) without s ?

@vsppedro
Copy link
Collaborator

Good question! should encrypt(:attr), without s, has my vote.

@matsales28
Copy link
Member

I'm fine either way. Let's use encrypt(:attr) then.

@theforestvn88 theforestvn88 force-pushed the add-encrypts-attributes branch from e8e58a0 to 17ff2c3 Compare December 27, 2023 07:47
@theforestvn88 theforestvn88 force-pushed the add-encrypts-attributes branch from 6762f8b to 68abf8f Compare December 28, 2023 03:03
Copy link
Collaborator

@vsppedro vsppedro left a comment

Choose a reason for hiding this comment

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

LGTM!

@matsales28 matsales28 merged commit 2a2b062 into thoughtbot:main Jan 2, 2024
15 checks passed
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.

4 participants