Skip to content
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

Feat new session alert #8315

Merged
merged 14 commits into from
Jul 3, 2024
Merged

Feat new session alert #8315

merged 14 commits into from
Jul 3, 2024

Conversation

loks0n
Copy link
Member

@loks0n loks0n commented Jun 24, 2024

No description provided.

@loks0n loks0n requested a review from Meldiron June 28, 2024 09:44
->label('sdk.response.type', Response::CONTENT_TYPE_JSON)
->label('sdk.response.model', Response::MODEL_PROJECT)
->param('projectId', '', new UID(), 'Project unique ID.')
->param('sessionAlerts', false, new Boolean(true), 'Set to true to enable session emails.')
Copy link
Contributor

Choose a reason for hiding this comment

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

enabled might be a better name here, since the scope is already session alerts

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked the same direction, but in other endpoints we do this too. like updateAuthLimit(limit) or updateAuthDuration(duration) or updateAuthStatus(status).
If you are against this pattern, we can make task in 1.6 QA board and discuss, but for now lets follow what is there.

With that in mind, I am changing it from updateSessionAlerts(sessionAlerts) to updateSessionAlerts(alerts).

app/init.php Outdated Show resolved Hide resolved
@lohanidamodar lohanidamodar merged commit 1c0490a into 1.6.x Jul 3, 2024
23 checks passed
@Meldiron Meldiron deleted the feat-new-session-alert branch July 3, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants