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

Add announcements #12662

Merged
merged 8 commits into from
Jan 23, 2020
Merged

Add announcements #12662

merged 8 commits into from
Jan 23, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Dec 22, 2019

Fix #11006

image

image

This PR introduces the ability for admins to publish announcements visible to local users only. The announcements are distributed through the streaming API (and available through the REST API). They can be scheduled for the future and be bound to a certain time range. Announcements can use the standard syntax for mentions, hashtags, URLs and custom emojis. The announcements, when present, are displayed atop the home feed. Users can add reactions to announcements in the form of unicode and custom emojis, and these reactions are distributed through the streaming API as well.

@Gargron Gargron added ui Front-end, design work in progress Not to be merged, currently being worked on moderation Administration and moderation tooling labels Dec 22, 2019
@Gargron Gargron force-pushed the feature-announcements branch 3 times, most recently from fe89bdf to e41ecf4 Compare December 23, 2019 01:01
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the inline comments, I have a few remarks/questions:

  • unlike for TLs and notifications, it seems that if streaming stops working at some point, new announcements will be missed entirely (no polling, nor, unless I'm mistaken, reconnecting to the streaming server?)
  • expired announcements seem to not disappear until the web app is restarted; some people typically never restart the web app, which would lead to an ever-growing list
  • judging by the code (please add screenshots/screencaps to UI changes), the announcements are displayed just above trending hashtags on single-column, which means it will only be displayed to users using single-column on a sufficiently-large screen. Furthermore, people have complained that trending hashtags were already getting cut off by the window's height, so it might be an issue too.
  • It seems to me that whenever an announcement appears, it will offset the selected announcement's index, which is a bit weird: showing neither the announcement that was being displayed, nor the newly-added one (unless the last displayed announcement was the most recent one)

app/controllers/api/v1/announcements_controller.rb Outdated Show resolved Hide resolved
app/lib/entity_cache.rb Show resolved Hide resolved
@Gargron Gargron force-pushed the feature-announcements branch 4 times, most recently from 0659828 to bbe10be Compare January 14, 2020 00:17
@Gargron Gargron removed the work in progress Not to be merged, currently being worked on label Jan 14, 2020
@Gargron Gargron marked this pull request as ready for review January 14, 2020 00:18
@Gargron Gargron force-pushed the feature-announcements branch 2 times, most recently from 03357de to d1cf11a Compare January 14, 2020 00:45
@Gargron
Copy link
Member Author

Gargron commented Jan 14, 2020

It seems to me that whenever an announcement appears, it will offset the selected announcement's index, which is a bit weird: showing neither the announcement that was being displayed, nor the newly-added one (unless the last displayed announcement was the most recent one)

Should the view always slide to the newest announcement when it arrives? Or should it always reset to the first one. Or keep showing the one that was previously being shown?

@Gargron
Copy link
Member Author

Gargron commented Jan 14, 2020

unlike for TLs and notifications, it seems that if streaming stops working at some point, new announcements will be missed entirely (no polling, nor, unless I'm mistaken, reconnecting to the streaming server?)

Would it really be worth it to poll the announcements API at the same rate as home/notifications?

@ClearlyClaire
Copy link
Contributor

unlike for TLs and notifications, it seems that if streaming stops working at some point, new announcements will be missed entirely (no polling, nor, unless I'm mistaken, reconnecting to the streaming server?)

Would it really be worth it to poll the announcements API at the same rate as home/notifications?

No, it would definitely not be worth polling at the same rate. Home/notifications are polled pretty often, announcements do not need to be refreshed nearly as often. I'd guess something as low as once every 15 minutes should be fine.

It seems to me that whenever an announcement appears, it will offset the selected announcement's index, which is a bit weird: showing neither the announcement that was being displayed, nor the newly-added one (unless the last displayed announcement was the most recent one)

Should the view always slide to the newest announcement when it arrives? Or should it always reset to the first one. Or keep showing the one that was previously being shown?

Hm, I'd say either keep to the current announcement and have some way to highlight there is a new one, or, just display the one that got pushed, as this shouldn't happen often anyway.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this seems ok except for the following facts:

  • once discarded, it doesn't seem possible to bring back announcements at all?
  • there is no interface or even API to create announcements, the only way is using the rails console…

app/controllers/api/v1/announcements_controller.rb Outdated Show resolved Hide resolved
app/models/announcement.rb Show resolved Hide resolved
@ClearlyClaire
Copy link
Contributor

Also, announcements in advanced mode look less obvious without the mascot
Screenshot_2020-01-14 Mastodon

end

ActiveRecord::Associations::Preloader.new.preload(records, :custom_emoji)
records
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about the performance implications of this…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, but this is a pre-optimization implementation and it might even end up being fine given the low number of announcements.

app/models/announcement_reaction.rb Show resolved Hide resolved
@Gargron Gargron force-pushed the feature-announcements branch 4 times, most recently from c1a67ee to 9212fa2 Compare January 16, 2020 17:21
@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Jan 16, 2020

Some of the remarks I have made earlier remain:

  • There is currently no way for anyone to write announcements, so while this PR is fine, merging it without the corresponding announcement authoring part would make little sense and only encourage implementing API support for a partially-implemented feature
  • it doesn't appear to refresh announcements when the websocket connection fails, while arguably this could be a perfect occasion to do so, if the websocket connection fails because of a server-side issue, per instance
  • expired announcements do not seem to be removed from the client state, meaning announcements would just pile up (they can be dismissed though so that's a very minor issue)
  • it seems newer announcements will offset the currently-viewed announcement, which is rather odd and won't necessarily inform a user that there are new announcements
  • there seem to be no way to view or bring back dismissed announcements

EDIT: admin interface to issue announcements added

@Gargron
Copy link
Member Author

Gargron commented Jan 19, 2020

  • expired announcements do not seem to be removed from the client state, meaning announcements would just pile up (they can be dismissed though so that's a very minor issue)
  • it seems newer announcements will offset the currently-viewed announcement, which is rather odd and won't necessarily inform a user that there are new announcements
  • there seem to be no way to view or bring back dismissed announcements

I would rather wait for real user feedback before tackling any of these, since I don't really know what people would expect. I have never wanted to bring a GitHub announcement back to look at it again, so this could just be wasting time unless there's real demand for it.

@brawaru
Copy link
Contributor

brawaru commented Jan 19, 2020

expired announcements do not seem to be removed from the client state, meaning announcements would just pile up (they can be dismissed though so that's a very minor issue)

When creating announcement, maybe there should be field for duration or something like that, I actually would like to come back to Mastodon after some time and see what changed on my lovely instance and with Mastodon itself (not for all the time all I was off (who'd be happy to see “1/9999…99”?), but at least some part of it), but announcements about planned maintenance have no practical sense for me.

there seem to be no way to view or bring back dismissed announcements

There is no point to bringing back specific announcement, but I see point in having announcements log, like blog, but log (haha, sorry…). Like for example, GitHub has changelog page where you can see all the recent changes and it is very simple, so must be announcements log, just another view (by view I mean… the same thing as if you clicked “Blocked users” menu item) and recent announcements of certain age (define age) or paginated, or infinitely scrollable (?). Link to it can be within the announcements panel, replacing instance domain, like “All announcements” or “View announcements”, or simplified “View all”.

Would that work?

@Gargron
Copy link
Member Author

Gargron commented Jan 19, 2020

When creating announcement, maybe there should be field for duration or something like that

There's ends_at

This can all be adjusted later

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Jan 20, 2020

  • expired announcements do not seem to be removed from the client state, meaning announcements would just pile up (they can be dismissed though so that's a very minor issue)
  • it seems newer announcements will offset the currently-viewed announcement, which is rather odd and won't necessarily inform a user that there are new announcements
  • there seem to be no way to view or bring back dismissed announcements

I would rather wait for real user feedback before tackling any of these, since I don't really know what people would expect. I have never wanted to bring a GitHub announcement back to look at it again, so this could just be wasting time unless there's real demand for it.

I do think being able to see dismissed announcements (at least for those that are still active) is needed, because should an announcement have some important or actionable information, users will be forced to either keep it (which does eat some significant screen space), or dismiss it and remember the important information anyway. Furthermore, this is kinda at odds with the reactions feature, since you'd be interested in seeing the reactions, but would also want to dismiss the announcement to reclaim some screen space.

EDIT: that being said, this can be addressed with user feedback, so we may want to merge that PR as is for now

@mayaeh
Copy link
Contributor

mayaeh commented Jan 20, 2020

I found some behavior that seemed to be malfunctioning. Please wait a bit for the merge due to questions.

@mayaeh
Copy link
Contributor

mayaeh commented Jan 20, 2020

I like this feature!

I applied this PR to the test server and tried it.
Then I found the following problem:

  • If the "Schedule publication" date and time are not set, "Published" appears to be displayed in UTC. ("Time range" is not UTC)
  • When "Time range" is set, the format displayed on the WebUI is not user-friendly depending on the language setting of server. (sample)
  • Even if you create a notification that specifies a future date and time in "Begin of event", it will be displayed once on the WebUI. But if you reload after a few minutes, it disappears.
  • When deleting announcement with added reaction, PublishAnnouncementReactionWorker: ActiveRecord::RecordNotFound: Couldn't find Announcement with 'id'=xx error is added to Sidekiq.
  • When the total number of Announcements to be displayed becomes 4, the close button and the next button disappear from the WebUI. And when it reaches 5, the back button and total number disappear. (sample1) (sample2)
  • You can create an announcement with All-day event checked without setting "Time range".

The following is future request:

  • The title displayed on the WebUI is "Update", but is "Announce" better?
  • I want a reset button to re-display the Announcement that was deleted by pressing the close button. I think that it is better to be able to cancel because there is a possibility of accidentally pressing.
  • If you press the close button to update the content of a deleted announcement, do you think it is better to re-display it?
  • If you try to add or delete a "reaction" to an expired or deleted announcement, we should return an error message telling you that the announcement has been deleted.
  • I want a screen that lists "Announcements". I also want a menu item to display that screen to "Getting-started".
  • If possible, I want the announcements updates and deletions also to be distributed via streaming API.

- Add `with_dismissed` param to announcements API
- Fix end date not being formatted when time range is given
- Fix announcement delete causing reactions to send streaming updates
- Fix announcements container growing too wide and mascot too small
- Fix `all_day` being settable when no time range is given
- Change text "Update" to "Announcement"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
moderation Administration and moderation tooling ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server annoucements
4 participants