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

Implement push notification #556

Merged
merged 6 commits into from
Apr 9, 2024
Merged

Conversation

gappc
Copy link
Collaborator

@gappc gappc commented Apr 7, 2024

@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

The checkbox component now emits a emit a "click" event.

In addition it visualizes if it is disabled.
This component sends a push notification to ODH, which in turn may forward it to other apps.
@gappc
Copy link
Collaborator Author

gappc commented Apr 7, 2024

Open topics to discuss:

  • the push button in the table view is disabled by default. At the moment, it is enabled only if the user is logged in. Should we restrict enabling the button further e.g. using roles?
  • 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?

@RudiThoeni
Copy link
Member

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
Copy link
Contributor

sseppi commented Apr 8, 2024

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
Copy link
Collaborator

@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
Copy link
Contributor

sseppi commented Apr 9, 2024

@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
Copy link
Collaborator Author

gappc commented Apr 9, 2024

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

@RudiThoeni
Copy link
Member

then lets merge it?

@sseppi
Copy link
Contributor

sseppi commented Apr 9, 2024

then lets merge it?

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

@RudiThoeni RudiThoeni marked this pull request as ready for review April 9, 2024 12:32
@RudiThoeni RudiThoeni self-requested a review April 9, 2024 12:32
@RudiThoeni RudiThoeni merged commit 113fc22 into development Apr 9, 2024
9 checks passed
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

4 participants