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

Fix access to the version history of organization-level roles #2654

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

Conversation

AndrewKvalheim
Copy link
Member

@AndrewKvalheim AndrewKvalheim commented Apr 3, 2020

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the upstream master branch.
  • The tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works(if appropriate).
  • I have added necessary documentation (if appropriate).

Short description of what this resolves, changes proposed

The tests of the version history of organization-level roles are currently failing. In trying to understand why, I encountered several complications:

  1. The tests expect the version history of organization-level roles to be accessible by conference organizers. This surprised me; I expected that organization administrators should have this access, not conference organizers.

    Proposed change: Test as an organization administrator.

  2. Regardless of that decision, currently neither organization administrators nor conference organizers have access, due to two independent bugs:

    1. No permission has been granted to access versions of organization-level roles.

      Proposed change: Grant permission.

    2. In the metadata of versions of organization-level roles, organization IDs are conflated with conference IDs; affiliation with organization n is recorded as affiliation with conference n. (Confusing name conference_id in versions table #1728)

      Proposed change: Distinguish between organization IDs and conference IDs.

  3. The tests that should have caught this return false negatives under SQLite. In this test configuration, the organization and conference happen to have numerically equal IDs, so their conflation goes undetected.

    Proposed change: I don't have any good ideas about how to detect this kind of mistake in the future. Maybe randomize IDs by prepopulating tables with filler rows? Or enforce unique IDs upon creation in factories?

I'm not confident about these changes. I don't fully understand the relationship between the is_admin flag and the organization_admin role, and I'm unclear about what the conference_id version metadata was supposed to be, e.g. should it be a polymorphic association instead?

Comment on lines 22 to 36
def deconflate
say 'deconflate'

PaperTrail::Version.where.not(conference_id: nil).where(item_type: %[Role UsersRole]).each do |version|
id = version.conference_id

if Organization.exists?(id)
raise "version #{version.id} conflates organization #{id} with conference #{id}" if Conference.exists?(id)

version.update_attributes conference_id: nil, organization_id: id
end
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

This is untested and needs discussion about how to handle the situation.

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #2654 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2654      +/-   ##
==========================================
+ Coverage   82.15%   82.16%   +0.01%     
==========================================
  Files         146      146              
  Lines        4780     4783       +3     
==========================================
+ Hits         3927     3930       +3     
  Misses        853      853
Impacted Files Coverage Δ
app/models/users_role.rb 100% <100%> (ø) ⬆️
app/models/role.rb 100% <100%> (ø) ⬆️
app/models/admin_ability.rb 93.47% <100%> (+0.03%) ⬆️

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 e709d08...fe2e5fe. Read the comment docs.

@AndrewKvalheim
Copy link
Member Author

The Travis CI build failed

Resolved by #2655.

AndrewKvalheim added a commit to AndrewKvalheim/osem that referenced this pull request May 31, 2020
AndrewKvalheim added a commit to AndrewKvalheim/osem that referenced this pull request May 31, 2020
@AndrewKvalheim AndrewKvalheim marked this pull request as draft August 20, 2021 17:28
This corrects the specification for the behavior of the Revision History
screen; changes to the organization administrator role should be
accessible to organization administrators, not conference organizers.
Without this permission, organization administrators can't see the
history of changes to the organization administrator role on the
Revision History screen.
Resolves failing test spec/features/versions_spec.rb:321

Partially reverts 81853d1
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.

1 participant