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

Changeset discussion feature #772

Closed
wants to merge 11 commits into from

Conversation

Projects
None yet
10 participants
@ukasiu
Copy link
Contributor

commented Jun 30, 2014

New ChangesetComment model and join table between users and changsets for comments subscriptions
Made commeting changesets possible via "browse" interface
Comments feed for all changsets, particular changeset
Email notifications of new comments on "subscribed to" changesets

tl;dr Changeset discussion feature
New ChangesetComment model and join table between users and changsets for comments subscriptions
Made commeting changesets possible via "browse" interface
Comments feed for all changsets, particular changeset
Email notifications of new comments on "subscribed to" changesets
@ukasiu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2014

@tomhughes Is this eventually going to be merged, should I provide more details? cc: @emacsen

@tomhughes

This comment has been minimized.

Copy link
Member

commented Jul 24, 2014

It's on my to do list - I just need to find a few hours to go through it.

@@ -58,6 +58,7 @@ def node_history
def changeset
@type = "changeset"
@changeset = Changeset.find(params[:id])
@comments = @changeset.comments.unscope(:where => :visible).includes(:author)

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

I think we should only remove the visibility restriction if the user is a moderator, or if we don't do that we should only render them in the view if the user is a moderator.

The class based client side show/hide stuff that you're using at the moment was only ever intended to be used where we couldn't (for caching reasons) do the restriction on the server, and that doesn't apply here (or anywhere really now as we have pretty much given up on caching).

On top of that, there may well be comments that we have to hide and which we really don't want showing up in the source at all even if the javascript does make them invisible.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Jul 29, 2014

Author Contributor

Will it be ok to assume that we're not showing hidden comments to non-moderators everywhere?

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 29, 2014

Member

I think so - I can't see why we would ever show hidden comments to non-moderators.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Jul 29, 2014

Author Contributor

I thought that caching was still an issue.

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 29, 2014

Member

It was only ever the diaries and trace lists that we tried to cache, and even that has been stopped now.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Aug 2, 2014

Author Contributor

Changed in cc26e21.

changeset = Changeset.find(params[:id])
render :text => changeset.to_xml.to_s, :content_type => "text/xml"
@changeset = Changeset.find(params[:id])
@comments = params['include_discussion'].presence ? @changeset.comments.includes(:author) : false

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

That looks bogus - you have a ternary operator with one branch returning an array and the other returning a boolean. Suggest the else should return an empty array, or perhaps more likely nil. In fact I'd suggest writing it in a much more readable way as:

@comments = @changeset.comments.includes(:author) if params[:include_discussion].presence

which will then be nil if the flag isn't set.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Aug 2, 2014

Author Contributor

Fixed in 28cee08.


# Find the changeset and check it is valid
@changeset = Changeset.find(id)
raise OSM::APINotFoundError unless @changeset

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

You shouldn't need this - the find call will raise ActiveRecord::RecordNotFound anyway if the id doesn't exist.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Aug 2, 2014

Author Contributor

I removed all these not found exceptions calls from the pull request.


# Find the changeset and check it is valid
@changeset = Changeset.find(id)
raise OSM::APINotFoundError unless @changeset

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

Same comment as above.


# Find the changeset and check it is valid
@changeset = Changeset.find(id)
raise OSM::APINotFoundError unless @changeset

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

Same comment as above.


# Find the changeset and check it is valid
@comment = ChangesetComment.find(id)
raise OSM::APINotFoundError unless @comment

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

Same comment as above.


# Find the changeset and check it is valid
@comment = ChangesetComment.find(id)
raise OSM::APINotFoundError unless @comment

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

Same comment as above.

xml.tag :k => k, :v => v
end
if @comments
xml.discussion do

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

Personally I would call this tag comments not discussion. Don't know what @zerebubuth thinks, as steward of the api?

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

It's an interesting question whether we should even have a container element I guess - we don't for the tags after all...

This comment has been minimized.

Copy link
@Zverik

Zverik Jul 25, 2014

Contributor

We discussed this, and "comments" would surely be confused with changeset comments. Of all possible names "discussion" seems to be the best option.

This comment has been minimized.

Copy link
@openstreetmap-website

openstreetmap-website Jul 25, 2014

Yes, this whole feature was renamed to "Changeset Discussions" because of
the comment confusion (see my talk at SOTM EU around this), but a
discussion contains multiple "comments"- though I guess we could change the
term to "post" or something that's less generic.

  • Serge

This comment has been minimized.

Copy link
@Zverik

Zverik Jul 30, 2014

Contributor

"Show discussion", "Subscribe to a discussion", "Add to discussion" etc :)

This comment has been minimized.

Copy link
@ukasiu

ukasiu Aug 8, 2014

Author Contributor

What is the better solution, to use XML from :read everywhere or to implement comments in to_xml?

I've taken it from notes and it probably wasn't the best idea. I will review the comments XML output.

Content-type and document type (like <?xml version="1.0" encoding="UTF-8"?>) is added automatically when using render with XML template.

Outdating by subsequent changes was the rationale for delaying comments.

This comment has been minimized.

Copy link
@emacsen

emacsen Aug 8, 2014

Contributor

Matt,

Some of these are implementation questions, but others appear to be
fundamental design questions. Please direct the design questions to me.

Find me on IRC.

  • Serge

This comment has been minimized.

Copy link
@zerebubuth

zerebubuth Aug 8, 2014

Contributor

I would have implemented comments by passing the :include_discussion flag to Changeset.to_xml and Changeset.to_xml_node, but I don't know if that's the better solution 😉. Equally, just unit testing the output of the builder against to_xml would ensure we stay in sync. The other benefit of implementing it in the to_xml methods is that it would then be trivial to include comments in the controller's :query and update methods, if anyone wants that.

About the document type - sorry, I didn't explain what I meant very well. Looking more closely, I see you're using the RichText helper, but hard-coding the type to "text". I think it would be useful to allow choice of that format by the commenter - I certainly find Markdown useful for linking and so forth.

About the outdating - that seems fine for now, although I think it's likely to be requested pretty quickly post-release 😄

This comment has been minimized.

Copy link
@tomhughes

tomhughes Aug 8, 2014

Member

Allowing markdown would be lovely, but we have the same problem as with notes - trying to render anything more sophisticated than text in the space available in the sidebar is kind of problematic.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Aug 10, 2014

Author Contributor

I've followed @zerebubuth suggestion to keep API implementation clear. I'm still wondering what text on "subscribe" button should say?

xml.user_url user_url(:display_name => comment.author.display_name, :host => SERVER_URL)

xml.text comment.body.to_text
xml.html comment.body.to_html

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

This (returning formatted HTML) doesn't really seem very api-ish to me?

This comment has been minimized.

Copy link
@ukasiu

ukasiu Jul 29, 2014

Author Contributor

Notes API returns HTML for note content.

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 29, 2014

Member

I know, but I was never very happy with it... The really crazy thing is that I don't think it's actually used anymore, but it was needed for the original notes implementation on the web site.

xml.instruct!

xml.osm(:version => API_VERSION, :generator => GENERATOR) do |osm|
osm << render(:partial => "changeset", :object => @changeset)

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

What's the point of extracting this into a partial? It doesn't look like it's used anywhere else?

@@ -0,0 +1,3 @@
<h2><%= t "changeset.rss.comment", :author => comments_entry.author.display_name,
:changeset_id => comments_entry.changeset.id.to_s %></h2>
<%= render :partial => 'comment', :object => comments_entry %>

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

What's the point of extracting this into a partial? It doesn't look like it's used anywhere else?

create_table :changesets_subscribers, id: false do |t|
t.column :subscriber_id, :bigint, null: false
t.column :changeset_id, :bigint, null: false
end

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 25, 2014

Member

We need to add indexes here on both subscriber_id and changeset_id or things will quickly collapse as we build up subscribers ;-) What I would suggest is a unique index on [:subscriber_id, :changeset_id] which will also make duplicate entries impossible, and an ordinary index on [:changeset_id] for finding the subscribers to a changeset.

This comment has been minimized.

Copy link
@pnorman

pnorman Jul 29, 2014

Contributor

My understanding is you want the more selective first in composite indexes, which probably means a unique index on [:changeset_id, :subscriber_id] and [:subscriber_id] for the other.

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jul 29, 2014

Member

My logic was that when we fetch subscribers for a changeset we won't care about the order, but when we fetch changesets for a subscriber we might - if for example we provided a paged list of changesets you were subscribed to.

This comment has been minimized.

Copy link
@ukasiu

ukasiu Aug 2, 2014

Author Contributor

Indexes added in 6a83a07.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Jul 25, 2014

I've done a pass through all the code now (haven't looked at tests yet) and put comments in line for your consideration.

In terms of UI issues there are a small number of things where I'd like to here what people think:

  • Subscribe button location - not sure I like it where it is but don't have better ideas at the moment.
  • Styling of the "login to comment" button.

There's also a major issue with the hide/unhide button - most people won't be able to see that but it looks horrible where it is. I suspect we need to make it a lot more subtle - maybe just a simple hide/unhide link at the end of the comment attribution line?

The other critical thing before we can merge is making sure everybody is happy with the API changes, which basically means:

  • Adding the include_discussion flag to the changeset API call.
  • The related changes to the XML for a changeset to include the comments.
@ukasiu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2014

I've made hide/unhide button a simple link as you suggested.

@tomhughes

This comment has been minimized.

The second index there is redundant - it should be on :changeset_id.

This comment has been minimized.

Copy link
Contributor Author

replied Aug 3, 2014

Yes, it was fixed in another commit.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Oct 21, 2014

I've finally managed to do a full review of this, and I think we're pretty much ready to do. I have a rebased tree with a few minor cleanups at https://github.com/tomhughes/openstreetmap-website/tree/next with a live version running at http://tomh.apis.dev.openstreetmap.org/.

There are two final questions I want to decide on before merging.

First is the RSS feeds for changeset comments - there is a per changeset feed at /api/0.6/changeset/NNN/comments/feed and a global feed at /api/0.6/changeset/comments/feed. The question I am asking myself is whether the global one is correct, or whether the word changeset should be plural, making it /api/0.6/changesets/comments/feed as it is in the main changesets feed URL?

In fact should the RSS feeds for changeset comments be under /api at all? None of the other RSS feeds are...

The second question is whether we want a separate permission flag for commenting - currently it requires an oauth token with full API write access before you can comment.

@emacsen

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2014

Tom,

First of all, THANK YOU. I believe this is the single most important new feature to the OSM website since notes. I expect this feature will help the OSM community communicate reduce conflicts, make vandalism easier to detect, and improve DWG communication with users. It's going to rock, and so I can't emphasize my gratitude enough to you for your help on getting this live.

I agree with you that the pluralization of the resources in the URI should be the same. I also agree with you that rss feeds under /api is odd. Do you want me to fix that or do you want to do it?

What's the alternative to the current permissions for commenting? If you're asking if they should be anonymous- I think the answer is no. If it's something else, would you mind elaborating?

@Zverik

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2014

RSS feeds should not be under /api — they are not part of API. I guess it won't be too hard to fix.
There is no /changesets currently at osm.org, but /history. How about /history/comments?
As for permission flag, why not, if those comments can be added through API.

@pnorman

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2014

The second question is whether we want a separate permission flag for commenting - currently it requires an oauth token with full API write access before you can comment

I'm inclined to keeping the api write separate from changeset comments - same as note creation is separate. I must admit, this isn't a very firm view, but it's how I'm leaning

@tomhughes

This comment has been minimized.

Copy link
Member

commented Oct 21, 2014

@emacsen I'm happy to make the changes - they're only minor. The alternative permissions wise is to have a separate permissionfor commenting on changsets, as we have a separate one for notes.

@Zverik The point of a separate permission would be that you could authorise an app to post comments without authorising it to modify any map data.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Oct 23, 2014

I've moved the changeset comment feeds in f636391 so that the new URLs are:

/changeset/NNN/comments/feed
/history/comments/feed

Derived from the location of other similar feeds in the namespace.

@dankarran

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2014

This seems like a great idea. One small improvement I'd suggest is to show the number of comments in the change set list so you can see at a glance any changesets which may be controversial.

screen shot 2014-10-28 at 20 41 34

Would it also be beneficial to list a user's changeset comments somewhere in their user profile?

@tomhughes

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

Can we concentrate on getting what we have merged before we start gilding the lily....

Which just means aking a decision about the permissions thing I think?

@pnorman

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2014

Minor issue, if a blocked user tries to comment on a changeset, it silently fails.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2014

I'm somewhat on the fence wrt the permissions (lot of good that does), I just don't quite see a likely use case (contrary to the same with notes). Do we actually have any noticeable use of the fine grained stuff at all? That is outside of the full-monty vs. accessing the user profile?

@tomhughes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2014

Most fall into either asking for everything, only asking for read_prefs (likely mostly people reading /api/0.6/user for authentication), or only asking for write_api.

Oddly the most common is actually asking for nothing at all, but I think we can discount those ;-)

@tomhughes

This comment has been minimized.

Copy link
Member

commented Nov 2, 2014

I'm delighted to be able to say that this was merged yesterday (as 2f22843) and has just been deployed.

I'd like to thank @ukasiu for his work on this, and @emacsen and @woodpeck for mentoring him, and offer my apologies for the time it has taken to get it merged.

@tomhughes tomhughes closed this Nov 2, 2014

@tyrasd

This comment has been minimized.

Copy link
Member

commented Nov 2, 2014

Awesome! But, I'm getting HTTP 403 (Forbidden) errors when commenting or subscribing on a changeset on osm.org with my account (the response body reads OAuth token doesn't have that capability.).

@tomhughes

This comment has been minimized.

Copy link
Member

commented Nov 2, 2014

Ah yes, forgot I needed to upgrade those.. Will do it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.