API - New endpoint to retrieve the articles in the reading list of the authenticated user

[Bhacaz]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

Add a new endpoint in the DEV API to retrieve the reading list articles for the authenticated user.

GET api/articles/me/readinglist

Related Tickets & Documents

This feature was requested in Issue #6755.

QA Instructions, Screenshots, Recordings

You can try the new API endpoint via a curl command with a valid api-key.

curl -H "api-key: API-KEY" http://localhost:3000/api/readinglist 

Added tests?

• yes
• no, because they aren't needed
• no, because I need help

Added to documentation?

• docs.forem.com/api
• readme
• no documentation needed

What gif best describes this PR or how it makes you feel?

[CLAassistant]

All committers have signed the CLA.

[benhalpern]

Big fan of this! It looks good at a glance but I'll circle back for a more comprehensive look soon.

[mstruve]

Pulled this down and tested it, works great!!!! Love this feature!

One small suggestion on default per_page size but otherwise LGTM!

[rhymes]

Hi @Bhacaz, great start! I have a couple of requests stemmed by the same argument: the reading list is and should be its own resource.

Preamble: the API (whose I stands for interface) doesn't necessarily need to reflect the organization of the code or of database models.

In this regard I think we should promote the "reading list" as its own collective resource: a collection of items. These items appear to be articles but they are indeed reactions. Those reactions have metadata that I believe it should be part of the serialization of the "reading list item" sent to the client.

In the context of the reading list, I think we should send back the date of creation as well.

For this reason I propose the following two changes:

• the URL should be /api/readinglist, simple as that :) /api/articles/me is a bit of a patch. In an ideal world we should just have a /api/articles/ and check authorization to choose which articles to send. Nesting the reading list under there is a bit odd.

• the serialized JSON should be something like (using pseudo json just to make this quicker :D):

[
  {
    "type_of": "readinglist",
    "created_at": "2020-10-03T04:49:16Z",
    "article": { 
    } 
   ...
  }
]

article then can be the rendering of the _article partial.

Let me know what you think, thank you!

[Bhacaz]

@rhymes

Thanks for the review and the comments.

the URL should be /api/readinglist

It's a good idea and if for any reason in the future we want to add some CRUD functions this resource will be at a good and significant place (URL).

Those reactions have metadata that I believe it should be part of the serialization of the "reading list item" sent to the client.

I agree, the payload should return some useful information about the reaction: created_at and maybe the status if someday handle archived elements from the reading list.

But I have some concern about placing the article inside an object in the response body. I think the users of the API will expert to receive directly the articles, not a reaction/readinglist. May I suggest the following serialized JSON:

{
  "type_of":"article",
  "id":18,
  "title":" The Wings of the Dove Inventore recusandae",
  "description":"Kickstarter normcore tattooed...",
  // Rest of the article attributes
  "readinglist": {
    "type_of": "readinglist"
    "status": "valid"
    "created_at": "2020-10-03T04:49:16Z",
  }
}

But I understand your point, the readinglist is in fact a list of reactions with an article reactable_type.

I let you have the last word. 😃

[rhymes]

@Bhacaz I'm imagining the use case which would be to build a list of favorite articles by a user, in theory by building an "hypertexted" API we shouldn't embed the article object but link to the API representation of it. As v0 is not "hypertexted" I think we can get away with embedding the article object. One of of the things I don't like about bookmarks is that I can't easily see when that bookmark has been added so I think metadata should be there, as we both agree. Great call about adding status.

What I'm less convinced about is to embed the full representation of the article object inside. I was thinking that the article endpoints are cached for a reason (serializing articles can be not super fast), so if we embed their exact representation inside the "list of saved items" (the reading list) we might want to eventually cache that as well (not necessary in this PR, we can adjust depending on real world performance).

If we look at the UI version of the reading list you get the following information:

The body is not there for example, nor are the stats. What if we adhere to that programmatically?

In conclusion:

• I think the serialization should make it super clear that this is a list of reading list items
• The article should be there but it doesn't necessarily should contain the entire representation of the index or show endpoints. Maybe we should add only the article data that would suffice to build a list of favorites

What do you think?

cc @benhalpern and @mstruve as well as they provided feedback earlier

[Bhacaz]

Change in 568dd93

• The readinglist have now his own resources.
• GET api/readinglist body return the information about the reaction and an embed article (the same information from GET api/articles), like it's was previously suggested
• I preload the maximum of records to use has must ActiveRecord::Relation#select has I can. I'm not sure if it's a commun pattern in this project.

[rhymes]

Thanks! Works well, left a few notes and corrections for the copy!

[Bhacaz]

@rhymes Thanks for the corrections, English is not my first language. 😃

If requested, I can squash all commits when the PR will be ready to merge.

[rhymes]

@Bhacaz thank you! Don't worry about that, commits will be squashed on merge :) Each PR corresponds to a single commit when we merge them in.

[mstruve]

This looks like its ready to go, we have a flaky spec that was just fixed in master. Can you update this branch and then we should get a green build and be good to go!

[Bhacaz]

6b9751e : Codeclimate found duplicated fragment of code...