Fix access to the version history of organization-level roles #2654
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewKvalheim
commented
Apr 3, 2020
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 |
There was a problem hiding this comment.
This is untested and needs discussion about how to handle the situation.
AndrewKvalheim
force-pushed
the
organization-role-versions
branch
from
857d0b2
to
fe2e5fe
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Resolved by #2655. |
This was referenced Apr 11, 2020
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
hennevogel
force-pushed
the
organization-role-versions
branch
from
fe2e5fe
to
e120a17
Compare
AndrewKvalheim
force-pushed
the
organization-role-versions
branch
from
e120a17
to
fe8e82a
Compare
AndrewKvalheim
force-pushed
the
organization-role-versions
branch
from
fe8e82a
to
dcbb689
Compare
AndrewKvalheim
force-pushed
the
organization-role-versions
branch
from
dcbb689
to
df883c9
Compare
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
AndrewKvalheim
force-pushed
the
organization-role-versions
branch
from
df883c9
to
3f6a9e9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
masterbranch.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:
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.
Regardless of that decision, currently neither organization administrators nor conference organizers have access, due to two independent bugs:
No permission has been granted to access versions of organization-level roles.
Proposed change: Grant permission.
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.
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_adminflag and theorganization_adminrole, and I'm unclear about what theconference_idversion metadata was supposed to be, e.g. should it be a polymorphic association instead?