Consistently use Version.visible scope #21716
Conversation
NobodysNightmare
force-pushed
the
visibility-consistency
branch
from
f2bb812 to
85bdc78
Compare
| let(:portfolio) { create(:portfolio, public: false) } | ||
| let(:inaccessible_project) { create(:project, public: false) } | ||
| let(:version) { create(:version, project:, sharing: "system") } | ||
| let(:version) { create(:version, project:) } |
There was a problem hiding this comment.
I find this surprising. Why would a spec that's designed to show that something might be inaccessibile define the tested version to use sharing: "system", which means that the version is shared system-wide?
There was a problem hiding this comment.
Apparently because it was never checked in the API :)
There was a problem hiding this comment.
Yes. That's what I am hoping for. The worse explanation is usually something like "it's intended, you just don't understand how/why".
But I agree, it probably just was never noticed.
|
The old and the new check is not really equivalent. The I'm not too sure if it makes sense to have a user with |
Thanks for the feedback. Yes this is true, though looking at the failing specs I didn't expect it to be so bad xD
Me neither, but on the other hand we allow for this in our permissions scheme 🤷 And at least one spec relied on it. |
NobodysNightmare
force-pushed
the
visibility-consistency
branch
2 times, most recently
from
763be76 to
863e3c1
Compare
Previously there was a separate permissions check for /api/v3/versions/:id. This check at least looked like it was inconsistent with the visible-scope. Using the scope both for /api/v3/versions and /api/v3/versions/:id ensures that both will return results consistent with each other. As part of this change, the manage_versions permission was also added into the Version.visible scope and the Version#visible? method, because it was missing so far from those places.
NobodysNightmare
force-pushed
the
visibility-consistency
branch
from
863e3c1 to
fa784d5
Compare
Previously there was a separate permissions check for /api/v3/versions/:id. This check at least looked like it was inconsistent with the visible-scope.
Using the scope both for /api/v3/versions and /api/v3/versions/:id ensures that both will return results consistent with each other.
References
Discovered as a side-find while working on #21661