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 Messages API #4605

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

milan-cvetkovic
Copy link
Contributor

@milan-cvetkovic milan-cvetkovic commented Mar 21, 2024

Copy link
Contributor

@tordans tordans left a comment

Choose a reason for hiding this comment

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

I don't know enough about he code but it looks like this likely duplicates some logic. Can this be shared in a concern or something similar?

app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
lib/oauth.rb Outdated Show resolved Hide resolved
@milan-cvetkovic
Copy link
Contributor Author

@tomhughes Friendly ping, any chance of reviewing the PR?

@milan-cvetkovic milan-cvetkovic force-pushed the message-api branch 3 times, most recently from f297cbf to 37c7ceb Compare May 24, 2024 16:04
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

I've had a first pass over the code and an initial peek at the tests and added my comments. I'll definitely want @gravitystorm to have a look as well before we merge.

app/controllers/api/messages_controller.rb Show resolved Hide resolved
app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/messages_controller.rb Show resolved Hide resolved
app/views/api/messages/outbox.json.jbuilder Outdated Show resolved Hide resolved
config/routes.rb Show resolved Hide resolved
config/routes.rb Show resolved Hide resolved

module Api
class MessagesControllerTest < ActionDispatch::IntegrationTest
def setup
Copy link
Member

Choose a reason for hiding this comment

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

Has this been copied from notes and not removed?

Copy link
Member

Choose a reason for hiding this comment

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

This still seems to be present? What was the resolution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I guess there were 2 mentions of notes, and I only removed one.

@milan-cvetkovic
Copy link
Contributor Author

I've had a first pass over the code and an initial peek at the tests and added my comments. I'll definitely want @gravitystorm to have a look as well before we merge.

Thanks for the review, will look into your comments shortly

@milan-cvetkovic
Copy link
Contributor Author

milan-cvetkovic commented Jun 13, 2024

Rebased and updated based on #4858.

@milan-cvetkovic
Copy link
Contributor Author

@tomhughes Please advise if any modifications are required for this PR, thanks.

app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
app/views/api/messages/_message.json.jbuilder Outdated Show resolved Hide resolved
app/views/api/messages/_message.json.jbuilder Outdated Show resolved Hide resolved
app/views/api/messages/_message.json.jbuilder Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved

module Api
class MessagesControllerTest < ActionDispatch::IntegrationTest
def setup
Copy link
Member

Choose a reason for hiding this comment

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

This still seems to be present? What was the resolution?

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

A few more minor comments...

@simonpoole
Copy link
Contributor

A comment on the generated output of the new API

"version": "0.6",
    "generator": "OpenStreetMap server",
    "copyright": "OpenStreetMap and contributors",
    "attribution": "http://www.openstreetmap.org/copyright",
    "license": "http://opendatacommons.org/licenses/odbl/1-0/",
...

I don't believe we would want to make a statement on the copyright of the messages in the first place (because they are essentially private and in any scenario you would need to agree with whoever you had communicated with on publishing them), but if it would be anything, the copyright would be held by the individual author of the respective e-mail.

@milan-cvetkovic
Copy link
Contributor Author

A comment on the generated output of the new API

"version": "0.6",
    "generator": "OpenStreetMap server",
    "copyright": "OpenStreetMap and contributors",
    "attribution": "http://www.openstreetmap.org/copyright",
    "license": "http://opendatacommons.org/licenses/odbl/1-0/",
...

I don't believe we would want to make a statement on the copyright of the messages in the first place (because they are essentially private and in any scenario you would need to agree with whoever you had communicated with on publishing them), but if it would be anything, the copyright would be held by the individual author of the respective e-mail.

This comes from including root_attributes in json and xml templates. I probably have it as a result of copying from other templates. However, it is not clear to me if we should include those headers in messages - your comment seems reasonable.

Side note - looking at notes (where I probably copied at least some templates from) I noticed that XML output does include the copyright notice, while json does not.

@milan-cvetkovic
Copy link
Contributor Author

@tomhughes I updated the code according to your suggestions from the last review.

Please advise if more modifications are necessary.

app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
app/controllers/messages_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/messages_controller.rb Outdated Show resolved Hide resolved
test/controllers/api/messages_controller_test.rb Outdated Show resolved Hide resolved
test/controllers/api/messages_controller_test.rb Outdated Show resolved Hide resolved
@milan-cvetkovic
Copy link
Contributor Author

@tomhughes code updated for comments from July 28.

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

Thanks. I think this all looks good now!

@tomhughes tomhughes merged commit f771938 into openstreetmap:master Jul 30, 2024
20 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.

6 participants