search

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
8 stars 7 forks source link

Implement an improved Push functionality #558

Closed sseppi closed 1 month ago

sseppi commented 2 months ago

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 https://github.com/noi-techpark/it.bz.opendatahub.databrowser/issues/556#issuecomment-2044542718

pkritzinger commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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

pkritzinger commented 2 months ago

@sseppi @gappc design has been updated: https://www.figma.com/file/DmuP6Dbv5LzkCNOXrDnWIL/2023-2024?type=design&node-id=2261%3A1911&mode=design&t=6G8f4lmqWbZTvKgH-1

gappc commented 2 months ago

@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 commented 2 months ago

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

sseppi commented 2 months ago

I like it too.

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

RudiThoeni commented 2 months ago

@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 commented 2 months ago

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

2) 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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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

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

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

gappc commented 2 months ago

@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:

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 commented 2 months ago

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 commented 2 months ago

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

RudiThoeni commented 2 months ago

thx ;)