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

[WIP] Notifications #1074

Closed
wants to merge 1 commit into from
Closed

[WIP] Notifications #1074

wants to merge 1 commit into from

Conversation

@jaxoncreed
Copy link
Contributor Author

I see that webhooks are in the codebase already, but I'm not sure if they are an old implementation or the new one. I've contacted @joachimvh on slack for clarification (it's late at night for him right now).

@joachimvh
Copy link
Member

I see that webhooks are in the codebase already, but I'm not sure if they are an old implementation or the new one.

I'll reply here so there's a paper trail. The notification format currently in the codebase is the old one with (unsecure) websockets. This was added to have consistent behaviour with NSS at that point.

@csarven
Copy link

csarven commented Nov 30, 2021

The Unofficial Draft (the proposal URL linked in the comment in this PR) is in the process of being removed from the proposals directory. The blob should still be accessible.

Canonical locations for CSS to track:

As mentioned in two Notifications Panel meetings, the use of a Well-Known URI is at risk. Needs an issue pending resolutions to:

Will follow up with updates to Notifications Protocol thereafter.

@ixuz
Copy link
Member

ixuz commented Nov 30, 2021

Hi there, we have actually started an implementation of the NotificationGateway, NotificationSubscription and WebHookSubscription2021 according to the specifications listed above. We intend to provide these as a Pull Request to this repository very soon.

Additionally, we discovered the need for more descriptive events fired upon resource changes in order to provide meaningful Notifications (Activitiy Streams 2.0). We'll open up a pull request today with that implementation. This will be useful for the specific implementations, such as the WebHookSubscription2021 and WebSocketSubscription2021.

@RubenVerborgh
Copy link
Member

@jaxoncreed Thank you for the pull request.
@ixuz Thank you for jumping in with code as well.

I think PRs #1074 and #1075 show something really important: we have to coordinate as a community when it comes to CSS.

Major features like these should not be implemented without having a joint architectural discussion. For most features, we can start this discussion in an issue here on GitHub. This is how we have always worked: in the open. For more complicated features, we might want to jump on a call. So let's do that 🙂

I will invite all of the people in this PR for a call, so we can see how to proceed together.

"args_handler": { "@id": "urn:solid-server:default:IdentityProviderParsingHandler" }
},
{
"comment": "Handles IDP input parsing.",
Copy link
Member

Choose a reason for hiding this comment

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

IDP stuff should very likely not be in the notifications configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a work in progress. I just copied over this configuration. Will clean up when it's done.

@jaxoncreed
Copy link
Contributor Author

jaxoncreed commented Nov 30, 2021

Thanks @RubenVerborgh for coordinating. @ixuz how soon do you think you'll make your PRs available. I'd love to help out.

@woutermont
Copy link
Contributor

@RubenVerborgh, I was already aware of @ixuz' progress on this, and would like to follow up the implemenation closely. Could you invite me to the call as well?

About coordinating ourselves as a community: #1079

@RubenVerborgh
Copy link
Member

@woutermont Absolutely; agree with your idea regarding roadmap, and you (and others) are most welcome to the meeting.

@jaxoncreed
Copy link
Contributor Author

This can be closed in favor of #1080

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