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

Authorize active_admin_comments in resource#show #5555

Merged
merged 6 commits into from
Nov 26, 2018

Conversation

amiuhle
Copy link
Contributor

@amiuhle amiuhle commented Nov 12, 2018

Check whether user is authorized to view comments when displaying them in resources#show.

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #5555 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5555      +/-   ##
==========================================
+ Coverage   98.38%   98.38%   +<.01%     
==========================================
  Files         293      293              
  Lines       11073    11078       +5     
==========================================
+ Hits        10894    10899       +5     
  Misses        179      179
Impacted Files Coverage Δ
features/step_definitions/comment_steps.rb 95.65% <100%> (+0.65%) ⬆️
...ive_record/comments/views/active_admin_comments.rb 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62d6f35...90029c4. Read the comment docs.

@deivid-rodriguez
Copy link
Member

@amiuhle We no longer maintain 1-3-stable, is this issue present in the master branch?

@amiuhle amiuhle changed the base branch from 1-3-stable to master November 14, 2018 15:43
@amiuhle
Copy link
Contributor Author

amiuhle commented Nov 14, 2018

@deivid-rodriguez I couldn't get all the tests to pass on master, so I tried on 1-3-stable.

I rebased on master and changed the target branch.

@deivid-rodriguez
Copy link
Member

Thank you! Could you add a test for this?

@amiuhle
Copy link
Contributor Author

amiuhle commented Nov 15, 2018

Added two feature specs.

Would you consider cherry-picking and releasing this as 1.3.2?

Edit: Maybe a fix for #5554 could also be addressed in a bugfix release.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

I added a few comments. Looking good!

features/comments/commenting.feature Outdated Show resolved Hide resolved
features/comments/commenting.feature Outdated Show resolved Hide resolved
features/comments/commenting.feature Outdated Show resolved Hide resolved
features/comments/commenting.feature Outdated Show resolved Hide resolved
@amiuhle
Copy link
Contributor Author

amiuhle commented Nov 22, 2018

@deivid-rodriguez I had changed the functionality of ListCommentsForCommenterOnly during development, instead of renaming it, I changed it again to match the class name.

Everything else is resolved, and I rebased a bit.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

@amiuhle I added a few minor comments about the specs. Thanks for your effort!

features/comments/commenting.feature Outdated Show resolved Hide resolved
features/comments/commenting.feature Outdated Show resolved Hide resolved
features/comments/commenting.feature Outdated Show resolved Hide resolved
features/comments/commenting.feature Outdated Show resolved Hide resolved
@amiuhle
Copy link
Contributor Author

amiuhle commented Nov 23, 2018

@deivid-rodriguez Using collection.where(author: user) now instead of finding the user of a specific email address. That was kind of stupid anyways :D

I did another rebase on master, so the changes you made in #5585 are gone.

Let me know whether you'd like me to rebase the changes from the last commit into the corresponding previous commits.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

@amiuhle I think it is fine as it is, thanks!

@deivid-rodriguez
Copy link
Member

If I was to cleanup the history of this PR, I would probably squash everything into one commit, up to you!

@amiuhle
Copy link
Contributor Author

amiuhle commented Nov 23, 2018

@deivid-rodriguez Should I add documentation? I think I could provide a PunditAdapter example.

Also, could you take a look at #5554?

@deivid-rodriguez
Copy link
Member

Sure, yes!

Copy link
Contributor Author

@amiuhle amiuhle left a comment

Choose a reason for hiding this comment

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

If I was to cleanup the history of this PR, I would probably squash everything into one commit, up to you!

I'll squash them. Just in case you didn't know this, you can do that via GitHub if you enable it in the repository settings: https://blog.github.com/2016-04-01-squash-your-commits/

@deivid-rodriguez
Copy link
Member

I'll squash them. Just in case you didn't know this, you can do that via GitHub if you enable it in the repository settings: https://blog.github.com/2016-04-01-squash-your-commits/

Yeah, I think we agreed a while ago to not enable this merge option and consistently use the standard merge, but I would personally enable it: it's handy and it helps in cases like this one.

@amiuhle
Copy link
Contributor Author

amiuhle commented Nov 26, 2018

@deivid-rodriguez I expanded on the existing pundit templates from the specs, since they're already referenced in the doumentation.

I'm not 100% happy with what the ActiveAdmin::CommentPolicy does vs how it's tested, but since the logged in user can't view any comments from other users, I didn't know how to test the delete? policy, for example.

I don't want to spend much more time on this PR, if you think it's okay like this, or have a good idea how to improve the last commit, I'll add some modifications. Otherwise I'll just remove the last commit and squash so you can merge this.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

@amiuhle I think it is ok like this! Squash and I'll reapprove!

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the help here! 👍🏻

@javierjulio javierjulio merged commit 585e593 into activeadmin:master Nov 26, 2018
@amiuhle amiuhle deleted the comment-authorization branch September 11, 2019 13:45
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