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

Remove hardcoded notifications capability #1805

Closed
pascalwengerter opened this issue Jun 21, 2021 · 5 comments · Fixed by #1808
Closed

Remove hardcoded notifications capability #1805

pascalwengerter opened this issue Jun 21, 2021 · 5 comments · Fixed by #1808

Comments

@pascalwengerter
Copy link
Contributor

Just found out that the ownCloud web frontend incorrectly checks for notifications since the notifications capability in Reva, for historical reasons and contrary to the behavior in other capability cases, returns an array with a "disable" string (

endpoints = ["disable"]
).

To make it consistent we shouldn't return anything here until Reva/oCIS is capable of handling notifications.

Glad to take care of this myself when the time comes but also don't want to cause a breaking change in the Semver for this minor nitpicky issue

@labkode
Copy link
Member

labkode commented Jun 21, 2021

Hi @pascalwengerter, what do you need? To omit the value or to set it to something else?

@pascalwengerter
Copy link
Contributor Author

Hi @pascalwengerter, what do you need? To omit the value or to set it to something else?

I'd vote in favor of omitting it since from my understanding that's how non-boolean values are being treated so far (?)

@labkode
Copy link
Member

labkode commented Jun 21, 2021

@pascalwengerter from the code looks like this capability is not a boolean.

   if h.c.Capabilities.Notifications == nil {
                h.c.Capabilities.Notifications = &data.CapabilitiesNotifications{}
        }
        if h.c.Capabilities.Notifications.Endpoints == nil {
                h.c.Capabilities.Notifications.Endpoints = []string{"list", "get", "delete"}
        }

The default would be an empty array.

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter from the code looks like this capability is not a boolean.

   if h.c.Capabilities.Notifications == nil {
                h.c.Capabilities.Notifications = &data.CapabilitiesNotifications{}
        }
        if h.c.Capabilities.Notifications.Endpoints == nil {
                h.c.Capabilities.Notifications.Endpoints = []string{"list", "get", "delete"}
        }

The default would be an empty array.

Fine with me, too. Checking for an array length feels way cleaner than doing if capabilities.notifications.endpoints[0] != "disable" (which even contains a grammatical error)

@labkode
Copy link
Member

labkode commented Jun 21, 2021

@pascalwengerter great, so we need the oc QA team to evaluate if changing the test to use an empty array will not break other things, I'll create a PR to check what goes wrong.

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 a pull request may close this issue.

2 participants