-
Notifications
You must be signed in to change notification settings - Fork 22
fix(tos banner): not show tos banner if there is no authentificated user #464
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ import { Store } from '@ngxs/store'; | |
|
|
||
| import { MockComponent } from 'ng-mocks'; | ||
|
|
||
| import { of } from 'rxjs'; | ||
|
|
||
| import { ComponentFixture, TestBed } from '@angular/core/testing'; | ||
| import { By } from '@angular/platform-browser'; | ||
|
|
||
|
|
@@ -16,13 +18,15 @@ import { TranslationServiceMock } from '@testing/mocks/translation.service.mock' | |
| import { OSFTestingStoreModule } from '@testing/osf.testing.module'; | ||
| import { provideMockStore } from '@testing/providers/store-provider.mock'; | ||
|
|
||
| describe('Component: Tos Consent Banner', () => { | ||
|
|
||
| describe('TosConsentBannerComponent', () => { | ||
| let component: TosConsentBannerComponent; | ||
| let fixture: ComponentFixture<TosConsentBannerComponent>; | ||
| let store: Store; | ||
| let store: jest.Mocked<Store>; | ||
|
|
||
| beforeEach(async () => { | ||
| await TestBed.configureTestingModule({ | ||
| imports: [OSFTestingStoreModule, TosConsentBannerComponent, MockComponent(IconComponent)], | ||
| imports: [TosConsentBannerComponent, OSFTestingStoreModule, MockComponent(IconComponent)], | ||
| providers: [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prune |
||
| provideMockStore({ | ||
| signals: [{ selector: UserSelectors.getCurrentUser, value: MOCK_USER }], | ||
|
|
@@ -32,7 +36,9 @@ describe('Component: Tos Consent Banner', () => { | |
| }).compileComponents(); | ||
|
|
||
| fixture = TestBed.createComponent(TosConsentBannerComponent); | ||
| store = TestBed.inject(Store); | ||
| store = TestBed.inject(Store) as jest.Mocked<Store>; | ||
| component = fixture.componentInstance; | ||
| store.dispatch = jest.fn().mockReturnValue(of(undefined)); | ||
| fixture.detectChanges(); | ||
| }); | ||
|
|
||
|
|
@@ -50,7 +56,6 @@ describe('Component: Tos Consent Banner', () => { | |
| }); | ||
|
|
||
| it('should dispatch AcceptTermsOfServiceByUser action when "Continue" is clicked', () => { | ||
| jest.spyOn(store, 'dispatch'); | ||
| const checkboxInput = fixture.debugElement.query(By.css('p-checkbox input')).nativeElement; | ||
| checkboxInput.click(); | ||
| fixture.detectChanges(); | ||
|
|
@@ -61,4 +66,25 @@ describe('Component: Tos Consent Banner', () => { | |
|
|
||
| expect(store.dispatch).toHaveBeenCalledWith(new AcceptTermsOfServiceByUser()); | ||
| }); | ||
|
|
||
| it('should return true for "acceptedTermsOfServiceChange" when user is null to not show banner', async () => { | ||
| await TestBed.resetTestingModule() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should just be pruned and just be the jest mock. Just make sure the mock is setup before the createComponent call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have tried the following approach and it does not set the
|
||
| .configureTestingModule({ | ||
| imports: [TosConsentBannerComponent, OSFTestingStoreModule, MockComponent(IconComponent)], | ||
| providers: [ | ||
| provideMockStore({ | ||
| signals: [{ selector: UserSelectors.getCurrentUser, value: null }], | ||
| }), | ||
| TranslationServiceMock, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| ], | ||
| }) | ||
| .compileComponents(); | ||
|
|
||
| const fixture = TestBed.createComponent(TosConsentBannerComponent); | ||
| const component = fixture.componentInstance; | ||
|
|
||
| fixture.detectChanges(); | ||
| expect(component.acceptedTermsOfServiceChange()).toBe(true); | ||
| }); | ||
|
|
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import { Button } from 'primeng/button'; | |
| import { Checkbox } from 'primeng/checkbox'; | ||
| import { Message } from 'primeng/message'; | ||
|
|
||
| import { Component, computed, signal } from '@angular/core'; | ||
| import { Component, computed, inject, signal } from '@angular/core'; | ||
| import { FormsModule } from '@angular/forms'; | ||
| import { RouterLink } from '@angular/router'; | ||
|
|
||
|
|
@@ -52,10 +52,16 @@ export class TosConsentBannerComponent { | |
| /** | ||
| * Computed signal indicating whether the user has already accepted the Terms of Service. | ||
| */ | ||
| readonly acceptedTermsOfServiceChange = computed(() => { | ||
| const user = this.currentUser(); | ||
| return user?.acceptedTermsOfService ?? false; | ||
| }); | ||
| acceptedTermsOfServiceChange = computed(() => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why no read only? That seems to be an Exoft pattern There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay |
||
| { | ||
| const user = this.currentUser() | ||
| /** | ||
| * if user is authenticated we check whether is accepted terms of service to hide banner or show if not | ||
| * otherwise user is not authenticated we hide banner always | ||
| */ | ||
| return user ? user.acceptedTermsOfService : true; | ||
| } | ||
| ); | ||
|
|
||
| /** | ||
| * Triggered when the user clicks the Continue button. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -221,7 +221,8 @@ | |
| "and": " and ", | ||
| "privacyPolicy": " privacy policy.", | ||
| "haveReadAndAgree": "I've read and agree to the terms and conditions", | ||
| "continue": "Continue" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is strange, how did this originally get committed? No need to respond. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I see we prefer to use en.json to text from it, though it may look redundant for small text. |
||
| "continue": "Continue", | ||
| "errorMessage": "We were unable to save your consent." | ||
| }, | ||
| "cookie-consent": { | ||
| "message": "Notice: This website relies on cookies to help provide a better user experience. By clicking accept or continuing to use the site, you consent to our use of cookies. See our Privacy Policy and Cookie Use for more information.", | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test for currentUser and not user?
What is returned when the user is not a user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test for currentUser and not user?I have added it to last commit
What is returned when the user is not a user?true - to not show tos banner if user is not loggined in