Enable feedback on allowed views

[eikichi18]

Previously, feedback could only be submitted on content. Now, it can also be enabled for specific views. A configurable whitelist has been introduced to define which views allow feedback.

[cekk]

Can you explain what's the new implementation in pr description? And maybe add some tests to validate it?

As i understand, now a "content" parameter is passed when adding a new feedback.
This should be a path that could be the actual context or the view that the user is about to evaluate (for example /search view). Right?

The only thing that i don't like with current implementation is that there isn't any check about the content, so someone could do a lot of POST with random paths and fill the db with a lot of garbage. Maybe could be a good idea adding some kind of whitelist (in p.a.registry?) of not-context paths that are allowed to be evaluated.

[mamico]

INavigationRoot is a IDexterityContent, are you sure there are no drawbacks with this change?

[eikichi18]

Yes, I'm sure. This change is breaking, so it requires a specific frontend version to be used.

[mamico]

If it is a breaking change, it requires a major version update and an explanation in the PR description and the README.

[eikichi18]

done

[mamico]

@eikichi18 is this a breaking change? Does it need to be released with a specific frontend version? Please explain that and document it in the README if necessary.

[eikichi18]

@cekk

As i understand, now a "content" parameter is passed when adding a new feedback.
This should be a path that could be the actual context or the view that the user is about to evaluate (for example /search view). Right?

Exactly

The only thing that i don't like with current implementation is that there isn't any check about the content, so someone could do a lot of POST with random paths and fill the db with a lot of garbage. Maybe could be a good idea adding some kind of whitelist (in p.a.registry?) of not-context paths that are allowed to be evaluated.

Generally speaking, yes, you're right. But in this case we have collective.honeypot that protects our endpoint from malicious intentions. Plone.app.registry might be a good idea but it might overcomplicated thing a little bit. In my opinion in terms of risk this does not represent any kind of cyber attack and constitutes a very low level of criticality.

[cekk]

honeypot prevent spam from bots, but you could see the POST call and make 1000 calls with for example /aaa/bbb/foo{n}
where n is 1-1000 and you get 1000 fake entries in the db.

It's the same thing if you call 1000 times on "/aaa/bbb/foo" but you're not breaking the user interface with 1000 rows. You'll have only one row with 1000 feedbacks and you can clean it with just one click.

[eikichi18]

@mamico @cekk

Done

[cekk]

I'm not convinced about the double management of "content" parameter: sometimes can be a path and sometimes a view name.
Why not using always a path?

[cekk]

Also readme should be updated with new parameter and the info that the endpoint should be called only on site root.

[eikichi18]

@cekk

Done also with README file