-
Notifications
You must be signed in to change notification settings - Fork 22
[ENG-9255] get proper data for Popular Pages chart rows #732
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
base: feature/pbs-25.03
Are you sure you want to change the base?
[ENG-9255] get proper data for Popular Pages chart rows #732
Conversation
…vant title using guid (title may be changed after saving in Elastic)
|
@mkovalua Fix unit tests and linting. |
| const all_attrs = { item_guid: resource?.id } as const; | ||
| const attributes = Object.fromEntries( | ||
| Object.entries(all_attrs).filter(([_, value]: [unknown, unknown]) => typeof value !== 'undefined') |
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.
What is a purpose of this code? Just pass guid same as before.
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.
To be it generic like it is implemented for ember also
also I have tried to investigate about _sessionId passing for count usage but not understand how it is possible to do properly because we have already the following
const expireDate = new Date('9999-12-31T23:59:59Z');
this.cookies.set(this.cookieName, 'true', expireDate, '/');
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.
I don't think you need it. Currently you are passing only 1 param.
| import { inject, Injectable } from '@angular/core'; | ||
|
|
||
| import { ENVIRONMENT } from '@core/provider/environment.provider'; | ||
| import { JsonApiService } from '@osf/shared/services/json-api.service'; |
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.
Revert this changes.
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.
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.
import { JsonApiService } from './json-api.service';
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.
Use such import, because I already did some changes there and will have conflict.
|
@mkovalua Fix unit test. You need to provide mock |
| beforeEach(async () => { | ||
| await TestBed.configureTestingModule({ | ||
| imports: [ProjectComponent, OSFTestingModule], | ||
| imports: [ProjectComponent, OSFTestingModule, NgxsModule.forRoot([])], |
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.
We don't use NgxsModule.forRoot. Review other tests and use approach with provideMockStore.

Purpose
The analytics tab for registrations is meant to help users understand how viewers are using and accessing their registration. One of those metrics is popular pages. However, as you can see in the image below, there are two of the same name, which makes it confusing to understand what viewers are looking at.
Summary of Changes
If we are clicking over navigation in details of registration and project title saving is as expected, if go to some tab by link
it return osf handle it to have proper title and render data in chart with row names showing what title tab was clicked
Screenshot(s)
98cf-7ac2-4806-aa08-935ab316dc33.mp4
Side Effects
it is rendered an old title name taken from Elastic if we update
maybe it will be worth to apply some backend logic to the newest title version or implement additional requests for title taking on rendering that may be time consuming
(have also tried to compare it with staging5 but for some reason popular pages is not rendering now
, locally I had some issues with docker elastic and M1 Max chip to compare
)
provider_id: routeMetricsMetadata.providerId,
action_labels: this._getPageviewActionLabels(routeMetricsMetadata),
client_session_id: this._sessionId,
not confident about correct usage #732 (comment)
QA Notes