Skip to content

Issue 335 update targeting event #352

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jan 5, 2021
Merged

Issue 335 update targeting event #352

merged 17 commits into from
Jan 5, 2021

Conversation

simzzz
Copy link
Contributor

@simzzz simzzz commented Dec 7, 2020 •
edited
Loading

closes #335

@simzzz simzzz changed the base branch from master to dev December 7, 2020 16:32
elpiel
elpiel reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

There was an Event that was no longer used iirc
can you double check if it's true and remove it?

sentry/src/access.rs
@@ -45,8 +47,10 @@ pub async fn check_access(
events: &[Event],
) -> Result<(), Error> {
let is_close_event = |e: &Event| matches!(e, Event::Close);
let is_update_targeting_event = |e: &Event| matches!(e, Event::UpdateTargeting { .. });
Copy link
Member

Choose a reason for hiding this comment

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

Do we allow only 1 event to update the targeting per request?

@simzzz simzzz marked this pull request as ready for review December 17, 2020 15:14
elpiel
elpiel requested changes Jan 4, 2021
View reviewed changes
Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

Looks almost ready 👍

Regarding testing and Redis, check this issue that can be tackled separately: #357

@simzzz simzzz requested a review from elpiel January 5, 2021 10:46
@elpiel elpiel mentioned this pull request Jan 5, 2021
Closed
3 tasks
@elpiel elpiel merged commit 501833f into dev Jan 5, 2021
@elpiel elpiel linked an issue Jan 7, 2021 that may be closed by this pull request
AIP#31 Event::UpdateTargeting (UPDATE_TARGETING) #335
Closed
5 tasks
@elpiel elpiel mentioned this pull request Jan 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elpiel elpiel elpiel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AIP#31 Event::UpdateTargeting (UPDATE_TARGETING)
2 participants
@simzzz @elpiel