noi-techpark / it.bz.opendatahub.databrowser

Explore and navigate through Open Data you need to build your next service.
https://databrowser.opendatahub.com
GNU Affero General Public License v3.0
9 stars 7 forks source link

Implement push notification #556

Closed gappc closed 7 months ago

gappc commented 7 months ago

@sseppi @mrabans @RudiThoeni @pkritzinger @Mazzolintis: This PR implements the push notification component as defined in #534

You can find a demo version here: https://9.databrowser.gappc.net/dataset/table/tourism/v1/ODHActivityPoi

Note that the column for the push functionality is the last one in the table

image

In order to take a look at the push responses, I've also added a config for them. A demo for the push responses can be found here: https://9.databrowser.gappc.net/dataset/table/tourism/v1/PushResponse

Please take a look at the PR and tell me your opinion

gappc commented 7 months ago

Open topics to discuss:

RudiThoeni commented 7 months ago

Hi @gappc Grea work! For the points to discuss on my view -I think enabling the button for the logged user should be enough..... the api then gives 401/403 etc... if the user is not allowed. Lets try this easy solution and if we see that we have to add role based button hiding we cann add it later... -For me this was clear, to close the popup and reopen it, for me as it works good...... but i let decide here stefano + the ux experts

On thing that came in my mind -Instead of showing the OPEN DATA HUB STATE in the table view i will change the config to show the "PublishedOn" published Channels, everyone agrees?

sseppi commented 7 months ago

Hi @gappc and @RudiThoeni

great work! I like it.

Hi @gappc Grea work! For the points to discuss on my view -I think enabling the button for the logged user should be enough..... the api then gives 401/403 etc... if the user is not allowed. Lets try this easy solution and if we see that we have to add role based button hiding we cann add it later...

I agree, let keep it simple. In case the real users get confused, we will then update. It is important that we expose a clear error description.

-For me this was clear, to close the popup and reopen it, for me as it works good...... but i let decide here stefano + the ux experts

Also here I agree with Rudi.

On thing that came in my mind -Instead of showing the OPEN DATA HUB STATE in the table view i will change the config to show the "PublishedOn" published Channels, everyone agrees?

Fine for me!

pkritzinger commented 7 months ago

@gappc awesome, thanks!

after a push is send, the "Send out the Push-Notifications" button is disabled. One needs to close the push-popup and then reopen it again to send another push. is this ok or should we add a "reset" button? If we don't want to have a "reset" button, should we add a description / text that explains how a user could do another push?

I also think that it's pretty straightforward, one optimization could be that we do not show the button in the disabled state anymore (user may think that (s)he needs to do sth. in order to reactivate the button) but only show the success msg?

what do @sseppi and @RudiThoeni think?

sseppi commented 7 months ago

@gappc @pkritzinger @RudiThoeni

As mentioned during the meeting, I think we can move this functionality in testing.

I have only small comments I would share with you:

  1. I suggest to change the text in the button from "Send Push Notification" to "Send Push"
  2. Once the Push has been sent, I would suggest to add in the cell of the Table View and the Popup the ID and the timestamp of the sent Push
  3. In case of error, we need to notify the user if the error is due to technical problems or missing rights.

I would suggest to close this PR and for my comments open a dedicated issue.

What do you think?

gappc commented 7 months ago

@sseppi yes, I agree, let me (or @RudiThoeni) know when the PR should be merged

RudiThoeni commented 7 months ago

then lets merge it?

sseppi commented 7 months ago

then lets merge it?

From my point of view yes, so I can show to our communication