-
Notifications
You must be signed in to change notification settings - Fork 11
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
(BEDS-561) adapt return struct #952
Conversation
Deploying beaconchain with Cloudflare Pages
|
@@ -253,7 +253,7 @@ func (d *DataAccessService) GetNotificationOverview(ctx context.Context, userId | |||
|
|||
func (d *DataAccessService) GetDashboardNotifications(ctx context.Context, userId uint64, chainIds []uint64, cursor string, colSort t.Sort[enums.NotificationDashboardsColumn], search string, limit uint64) ([]t.NotificationDashboardsTableRow, *t.Paging, error) { | |||
// dev hack; TODO remove |
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 would quickly explain why this was necessary, but the most important part is under which circumstances
this can be removed
e.g. during development
of Ticket BEDS-565
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.
Resolved via #956 (requires is_mock_enabled
to be set)
frontend/types/api/notifications.ts
Outdated
validator_offline: number /* uint64 */[]; // validator indices | ||
group_offline: NotificationEventGroup[]; // TODO not filled yet | ||
proposal_missed: IndexBlocks[]; | ||
group_offline: string[]; // TODO not filled yet |
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 do not get why this type is broader
now?
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.
As stated above: because the dashboard name is a static part of the request and doesn't have to be part of each list item (same for group name; also see internal discussion)
proposal_done: IndexBlocks[]; | ||
upcoming_proposals: IndexBlocks[]; | ||
upcoming_proposals: IndexSlots[]; |
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.
love this thx 💪
np: |
e06bcb7
to
8e1f3e2
Compare
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.
lgtm, just one question.
if notificationDetails.GroupName == "" { | ||
notificationDetails.GroupName = t.DefaultGroupName | ||
} | ||
|
||
buf := bytes.NewBuffer(result) | ||
gz, err := gzip.NewReader(buf) | ||
if err != nil { | ||
return nil, err | ||
if notificationDetails.DashboardName == "" { | ||
notificationDetails.DashboardName = t.DefaultDashboardName | ||
} |
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.
question: Why do we set the names to default here and don't leave them as empty strings?
It should also be impossible to create a dashboard or group without a name with at least length 1.
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.
You're right, it's more of a debug/testing thing - some internal dashboards don't have a name so I thought it wouldn't hurt
Added the
dashboard_name
, moved it to the root of the struct and removed all references todashboard_id
as the entire request is dashboard specific already