Closed nfmohit closed 2 months ago
IB :white_check_mark:
Hi @nfmohit, sorry to only spot this now, but while reviewing the PR for this issue I've realised that taking this approach would lead to some problems.
Specifically, in the case where the currently selected publication has been deleted, no user would pass the test for module access. Therefore:
recover-modules
endpoint.Maybe I've missed something, but it looks like we should reconsider the approach. Please take a look and see what you think.
Thank you for raising these concerns, @techanvil! I spoke with @aaemnnosttv about this as well and we think this would not be a problem.
- We wouldn't actually know if the user has permission to access the module in order to change the selected publication.
We use the hasModuleOwnershipOrAccess
selector to determine if the user can change the selected publication. This will always return true
for the module owner. Hence, they can change the publication in this case.
- If the module lost its owner and became recoverable, no user would be able to recover the module due to the check for module access in the
recover-modules
endpoint.
At this stage, RRM will not appear as a recoverable module as it is not shareable. If in the future this module does become shareable, we will have to think about this further. I'll add this to my notes.
Please let me know what you think, thank you!
Thanks @nfmohit. That's a great point about using hasModuleOwnershipOrAccess()
so we determine access via the owner rather than the entity access. And, I managed to completely overlook the sharing aspect with regard to recoverable modules :facepalm:.
So, the code is totally fine as specced and implemented. Thanks to you and @aaemnnosttv for clarifying, sorry for the false alarm!
I've executed the steps and when it comes to login under another Google account under another browser, I can still see the Publication select.
The QAB states:
Verify that the publication select does not appear.
In my case, I get the publication select(dropdown) and there is value, albeit it's disabled. My expectation was for the dropdown not to appear?
It could be just a case of the QAB needing an update also.
Screenshot and video for reference below:
Hi @kelvinballoo. You are right. With #9151 merged, the QAB was outdated. I have updated it. Please let me know if you have any other questions, thank you!
Thanks for confirming @nfmohit
This is verified good. Moving ticket to Approval.
Feature Description
Currently, the
Modules\Reader_Revenue_Manager::check_service_entity_access()
method only checks if the current user can make alist
request to the SwG API, which technically does not dictate if they have access to an actual publication.This method should be updated to also include checks to see if the current publication exists in the said
list
response.This should be similar to
Modules\Tag_Manager::check_service_entity_access()
.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
Modules\Reader_Revenue_Manager::check_service_entity_access()
method:listPublications
call should be captured in a new variable, e.g.$publications
.true
unconditionally after the try/catch block, the method should returntrue
only if a publication exists in$publications
with the savedpublicationID
module setting.Modules\Tag_Manager::check_service_entity_access()
for inspiration.Test Coverage
tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php
:Tests\Modules\Tag_ManagerTest
for inspiration.QA Brief
rrmModule
feature flag.Changelog entry