Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

svcid checks for enabling notifications #152

Merged
merged 7 commits into from
Jul 29, 2020

Conversation

Blackbaud-TerryHelems
Copy link
Contributor

Logic:
Showing the bell in the the LE/ENV context:
If the org has notif AND (renxt or fenxt) » show the bell

Showing the bell when there is no LE context:
If svcid == skydev » show the bell

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #152 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #152   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines         1109      1114    +5     
  Branches       186       188    +2     
=========================================
+ Hits          1109      1114    +5     
Impacted Files Coverage Δ
src/omnibar/omnibar.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a52244...6d13572. Read the comment docs.

const CLS_EXPANDED = 'sky-omnibar-iframe-expanded';
const CLS_LOADING = 'sky-omnibar-loading';

const notificationSvcIds: any = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would either create an interface for this or declare a type inline:

const notificationSvcIds: {
  [key: string]: {
    requiresNotif: boolean
  }
} = {
  ...
}


fireMessageEvent({
messageType: 'get-token',
tokenRequestId: 123
});
});

describe('handle notifications per svcid', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nested describe should follow the parent describe logically as if it were a sentence, so since the outer describe is "Omnibar," maybe this describe should be "when connecting to notifications," then describe the action in the it blocks.

});
}

it('renxt', (done) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it blocks start with "should" and describe the actions under test. I would combine these 4 tests into 1 and call it "should respect the notification settings for a given service ID."

@Blackbaud-PaulCrowder Blackbaud-PaulCrowder merged commit 5f81d1c into master Jul 29, 2020
@Blackbaud-PaulCrowder Blackbaud-PaulCrowder deleted the notification-svcid-checks branch July 29, 2020 18:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants