Add RRM module settings. #8965
Merged
Add RRM module settings. #8965
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
…ement/8793-rrm-module-settings.
…ement/8793-rrm-module-settings.
…ement/8793-rrm-module-settings.
Base automatically changed from
enhancement/8791-get-publications-endpoint
to
develop
July 5, 2024 22:04
|
Build files for 063a2e9 have been deleted. |
|
Size Change: +116 B (+0.01%) Total Size: 1.57 MB
ℹ️ View Unchanged
|
techanvil
requested changes
Jul 9, 2024
tests/phpunit/integration/Modules/Reader_Revenue_Manager/SettingsTest.php
Show resolved
Hide resolved
tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php
Outdated
Show resolved
Hide resolved
tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php
Outdated
Show resolved
Hide resolved
…gle/site-kit-wp into enhancement/8793-rrm-module-settings.
nfmohit
reviewed
Jul 17, 2024
| */ | ||
| public function on_deactivation() { | ||
| $this->get_settings()->delete(); | ||
| $this->reset_data_available(); |
There was a problem hiding this comment.
The Reader Revenue Manager module, as it currently stands, does not have any data to work with, hence, it should neither use the Module_With_Data_Available_State_Trait trait, nor call the reset_data_available function.
Is there a specific reason this was added? If not, I'm happy to remove this as part of one of my upcoming PRs.
There was a problem hiding this comment.
Good spot @nfmohit 🦅 👁️! I missed this in CR. I believe this should indeed be removed, if you can do it via one of your forthcoming PRs that would be great.
aaemnnosttv
reviewed
Jul 26, 2024
| protected function get_sanitize_callback() { | ||
| return function ( $option ) { | ||
| if ( isset( $option['publicationID'] ) ) { | ||
| if ( ! preg_match( '/^[a-zA-Z0-9]+$/', $option['publicationID'] ) ) { |
There was a problem hiding this comment.
@ankitrox This is inconsistent with the related check we have in JS and does not allow for a valid ID that I am trying to test with. See #8948 (comment)
@techanvil
There was a problem hiding this comment.
Thanks for spotting this @aaemnnosttv, sorry I missed it during CR. As mentioned on Slack, I've got a followup PR ready if we want to reopen this issue, otherwise I'll create a separate issue for the fix.
There was a problem hiding this comment.
Update: The above PR has now been merged.
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.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist