-
Notifications
You must be signed in to change notification settings - Fork 286
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
Handle unavailable publication scenario in RRM settings edit screen #9151
Comments
@aaemnnosttv This has been added based on our discussions. I'm looking for your opinion in the ACs and its review. Some questions:
Thank you! |
Thanks @nfmohit – we should avoid using language that makes definitive statements about there being no publications in the users account since we can't know that without making a different request which isn't worth it just to inform a message. There very well could be many other publications, but we are limited to 1) publications the user has access to, and 2) those that have a matching verified domain. Is it possible to change the domain a publication is verified for after it is verified for a given domain? In the Settings Edit view, we need to keep the current publication ID in state/settings until it can be replaced by a new one (otherwise submit changes would fail validation). This is partly the reason for the "has access" check with locking behavior for non-module-owner admins, so that is another nuance to consider. The scenarios defined so far only really apply to the module owner as non-owners shouldn't be able to edit the connected publication if they didn't have access. Let's look at how we handle this for other modules as I'm guessing we should have something similar for GA and GTM. The difference here is that we don't offer the creation via the dropdown which simplifies this case for those modules, but let's check the other modules as I think we can align the language more closely with those as well. |
Thank you for the kind review, @aaemnnosttv !
It is possible to change the domain a publication is verified for, yes. As discussed on our call, I've updated the ACs accordingly and also added an IB for your consideration. Thank you! |
Thanks @nfmohit AC + IB ✅ |
…blication Update RRM settings screens to handle edge case scenarios
QA Update
|
Hi @kelvinballoo. Unfortunately, there isn't an easy way to simulate this, but you can see the scenario in this Storybook story. Please let me know if this sounds good, thank you! |
QA Update ✅Was able to do the above scenario by having another admin delete the original admin. The correct notice was shown then: These were tested good as well:
|
Feature Description
There could be a number of scenarios where the connected publication is no longer available in the RRM settings edit screen. Such as:
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.js
:getPublicationID
) does not exist in the list of publications (getPublications()
), render anErrorNotice
component with the message specified in the ACs.PublicationSelect
as it is done currently.PublicationOnboardingStateNotice
only if the user has module access.SettingsNotice
with the notice mentioned in the ACs, with props similar toassets/js/modules/analytics-4/components/settings/AnalyticsSettingsNotice.js
.assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.stories.js
:Test Coverage
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.test.js
:QA Brief
rrmModule
feature flag.Changelog entry
The text was updated successfully, but these errors were encountered: