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 an improved Push functionality #558

Closed
sseppi opened this issue Apr 9, 2024 · 17 comments
Closed

Implement an improved Push functionality #558

sseppi opened this issue Apr 9, 2024 · 17 comments

Comments

@sseppi
Copy link
Contributor

sseppi commented Apr 9, 2024

As mentioned in the PR I open a new issue to collect the improvements of the push notification that we published in testing.

The improvement that emerged are the following.

  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.

As example of information about the pushes we have I post here an example:
https://databrowser.opendatahub.testingmachine.eu/dataset/raw/tourism/v1/PushResponse/06f5bbe2-3b5f-4ab0-8c27-0e1fa5879f38

I think that the most interesting information for the final user are Date and ID

@pkritzinger: what is your feedbcak?

Originally posted by @sseppi in #556 (comment)

@sseppi sseppi added this to the Data Browser 2.5 - Beta milestone Apr 9, 2024
@pkritzinger
Copy link
Collaborator

@sseppi looks good from my Point of View - let's do it!
@gappc plase let me know if you see any technical obstacles. As soon as we have your GO, we can add these features to the design and share it with you.

@gappc
Copy link
Collaborator

gappc commented Apr 9, 2024

@sseppi @pkritzinger, here is my feedback:

  1. I suggest to change the text in the button from "Send Push Notification" to "Send Push" => no problem
  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 => there we would have to load the last timestamp for each row in the table from the PushResponse endpoint, I suspect that sooner or later there will be performance problems. Alternative: show the last push timestamp ONLY in the popup
  3. In case of error, we need to notify the user if the error is due to technical problems or missing rights. => that should be possible, I just need to know how those errors should be shown to the user

@sseppi
Copy link
Contributor Author

sseppi commented Apr 10, 2024

@gappc @pkritzinger

  1. 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 => there we would have to load the last timestamp for each row in the table from the PushResponse endpoint, I suspect that sooner or later there will be performance problems. Alternative: show the last push timestamp ONLY in the popup

Fine for me to show the push data only in the popup.

@pkritzinger
Copy link
Collaborator

@gappc @sseppi
sounds good, we'll provide a design for next call.

@pkritzinger
Copy link
Collaborator

@gappc
Copy link
Collaborator

gappc commented Apr 21, 2024

@sseppi @mrabans @RudiThoeni @pkritzinger @Mazzolintis all three tasks are implemented as discussed in PR #561.

You can find the updated implementation deployed here: https://9.databrowser.gappc.net/dataset/table/tourism/v1/ODHActivityPoi

Please check it and let me know what you think of it

@pkritzinger
Copy link
Collaborator

@gappc only checked frontend and like it a lot :)

@sseppi
Copy link
Contributor Author

sseppi commented Apr 22, 2024

I like it too.

@RudiThoeni I think we can merge the PR quickly test it and then move it also to production.

@RudiThoeni
Copy link
Member

@gappc i deployed anything in production, and now i added the pushbutton on the listview of Articles etc... on development

I activated now two publishers for Push

On the Articles View (https://databrowser.opendatahub.testingmachine.eu/dataset/table/tourism/v1/Article?articletype=newsfeednoi) i noticed that there is always the "Idm Marketplace" visible even if there is no Published-On idm-marketplace present.
image

On an Article with PublishedOn (noi-communityapp) also the Idm-Marketplace appears and the push is effectively done to idm-marketplace.....
image

The channels listed should be like in the publishedOn field
-If no Publisher present no channel visible (cannot send push)
-If more Publishers present more channels visible

Hope i deployed everything right, can you please check this two behaviours........

thx and cheers

@RudiThoeni
Copy link
Member

I am little bit confused, cannot remember exactly if we specified this functionality this way

  1. Show always all push channels independently from what publisher the record has...... and let the api decide if it is pushed or not

OR

  1. Show only the publisher that is assigned as push channel. (What i mean is if the record has publisher idm-marketplace assigned and idm-marketplace has a push channel --> we show it and let push. If a record has noi-communityapp assigned as publisher and noi-community has a push channel defined -- we show it)

What do you think? The cool thing is we have this publisher -> push -> publishedOn combination..... so option 2) should be possible.....

@gappc
Copy link
Collaborator

gappc commented Apr 23, 2024

@RudiThoeni maybe I misunderstood the meaning of the PublishedOn attribute, I've replied to your mail

In the mail I forgot to mention, that the issue with noi-community-app not showing up should be resolved with PR #567, could you please take a look at it? thx :)

@RudiThoeni
Copy link
Member

@gappc looks good now every pushchannel is listed and the user can select where i wants to push.
I will add a check on Backend Side if the Pushed Object is valid (I mean if it has the PublishedOn Attribute set to the right channel)

So i think the best is let us start with the functionality as it is and we will discuss if we add the improvements on the UI mentioned by me.

@RudiThoeni
Copy link
Member

RudiThoeni commented Apr 24, 2024

Everything is on production I did some tests and added some points to discuss.
I like the result it seems very clear and straightforward

  • Unauthorized User (a 403 is produced)
image

to discuss maybe here we could hide the button like the edit button

  • Push of data which is not activated for this channel (PublishedOn)
image
  • Push that worked
image
  • Multiple Push where one succeeded the other is not allowed
image

Let's use it as it is now and add some optimizations after a testingperiod....

@gappc
Copy link
Collaborator

gappc commented Apr 26, 2024

@RudiThoeni @sseppi I've improved the visualization using the feedback from @RudiThoeni to make the whole process for the user (hopefully) more straight-forward.

The send-push button is enabled only if:

  • the user is logged in
  • "PublishedOn" for the given record has values
  • there are Publishers whose "PushConfig" contains values
  • the intersection between "PublishedOn" un Publishers is not empty

If all of the cases above are true, then publishers shown in the popup are those from the intersection between "PublishedOn" un Publishers

You can give it a try at e.g.

@RudiThoeni
Copy link
Member

Hi @gappc

Perfect great work, exactly what i wanted ;)
We can put it into testing + production please open a PR

thx and cheers
Rudi

@gappc
Copy link
Collaborator

gappc commented Apr 30, 2024

Hey @RudiThoeni, here you go: PR #570 ;)

@RudiThoeni
Copy link
Member

thx ;)

@sseppi sseppi closed this as completed May 14, 2024
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

No branches or pull requests

4 participants