Update logic for publishers #7439
Conversation
Auditors: @mrose17, @bsclifton Fix #7089 Fix #7429
| // If session is clear then siteSettings is undefined and icon | ||
| // will never be shown, but synopsis may not be empty. | ||
| // In such cases let's check if synopsis matches current publisherId | ||
| return !!this.props.synopsis.map(entry => entry.get('site')).includes(this.publisherId) |
There was a problem hiding this comment.
Any reason this changes from the regular .includes() to a double negate using !!? The double negate is great for making something is truthy (when you don't care about it's value), but in this case includes() should already be returning a bool
| const autoSuggestSites = getSetting(settings.AUTO_SUGGEST_SITES) | ||
|
|
||
| // hostSettings is undefined until user hit addFunds button. | ||
| // For such cases check autoSuggestSites for eligibility. | ||
| return this.hostSettings | ||
| ? this.hostSettings.get('ledgerPayments') !== false |
There was a problem hiding this comment.
Do all hostSettings entries have a ledgerPayments field? because comparing !== false will be true if that field is undefined. you might want to change this to something like:
return this.hostSettings && typeof this.hostSettings.get('ledgerPayments') === 'boolean'
? this.hostSettings.get('ledgerPayments') !== false
: this.validPublisherSynopsis || (autoSuggestSites && !excluded)
There was a problem hiding this comment.
yep that's intentional. Ledger only recognizes false values. So if it's not on payment list yet (no user action), field will be undefined. If other conditionals don't apply then we consider it's included for payments.
| this.authorizedPublisher && this.verifiedPublisher && styles.fundVerified, | ||
| !this.authorizedPublisher && !this.verifiedPublisher && styles.noFundUnverified, | ||
| this.authorizedPublisher && !this.verifiedPublisher && styles.fundUnverified | ||
| !this.enabledForPaymentsPublisher && this.verifiedPublisher && styles.noFundVerified, |
There was a problem hiding this comment.
if we wanted to make this cleaner, you could do the && for the two conditions above and store in a const, like
const showNoFundVerified = !this.enabledForPaymentsPublisher && this.verifiedPublisher
// ...
css(
showNoFundVerified && styles.noFundVerified,
)
There was a problem hiding this comment.
Agree. Will take this for next refactors and just updated Wiki for future reference. Thanks
| exclude: boolean, // wheter or not site is in the excluded list | ||
| stickyP: boolean, // wheter or not site was added using addFunds urlbar toggle | ||
| timestamp: number // timestamp in milliseconds | ||
| } |
There was a problem hiding this comment.
super small nitpick- would be nice to alphabetize these fields. Sublime has this sorting built in; I believe you'd need an add-on for VS Code (cc @NejcZdovc)
There was a problem hiding this comment.
Yeah, we should keep all properties in the state.md sorted alphabetically, so that we can easily read through it
git rebase -ito squash commits (if needed).Auditors: @mrose17, @bsclifton
Fix #7435
Fix #7089
Fix #7429
Test Plan:
Despite when autoSuggest is OFF, all steps covered by automated test:
QA Steps
addFunds toggle is shown by default when
addFunds toggle is shown as enabled by default when
addFunds toggle is shown as verified when
addFunds toggle is set to ON when
addFunds toggle is set to OFF when