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

Change notification settings to new API #181

Merged
merged 1 commit into from Jan 31, 2023

Conversation

phylor
Copy link
Collaborator

@phylor phylor commented Jan 27, 2023

There has been some changes to the notification settings API. This PR accomodates for that.

The old API returned an object, such as:

{
  data: {
    ride_notifications: ['sms']
  }
}

Now, the API returns an array of NotificationSetting:

{
  data: [
    {
      id: 'ride_notifications',
      name: 'ride_notifications',
      channels: ['sms']
    }
  ]
}

@phylor phylor force-pushed the feature/notification-settings-2.0 branch from ae93ad3 to c3b0800 Compare January 27, 2023 13:21
@phylor phylor marked this pull request as ready for review January 27, 2023 13:23
@phylor
Copy link
Collaborator Author

phylor commented Jan 27, 2023

@tom-ioki It would be great to get your opinion on this PR. I had to hack around a bit to allow an API response to be an Array instead of a Hash. It works, but it looks hacky and it doesn't support etags when an array is returned.

@tom-ioki
Copy link
Contributor

@tom-ioki It would be great to get your opinion on this PR. I had to hack around a bit to allow an API response to be an Array instead of a Hash. It works, but it looks hacky and it doesn't support etags when an array is returned.

Hey 👋
sorry for the late reply. It took me a while to understand the issue. Maybe we should have a short call?

Copy link
Contributor

@tom-ioki tom-ioki left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

As discussed, I will create a follow up issue for this. Maybe we can move this logic into the model world and use some kind of keyword which indicates that the response will be an array of hashes or something like that.

@phylor phylor merged commit 270eeff into main Jan 31, 2023
@phylor phylor deleted the feature/notification-settings-2.0 branch January 31, 2023 10:28
@tom-ioki tom-ioki mentioned this pull request Jan 31, 2023
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

2 participants