Cloudkibo / CloudKibo

CloudKibo
0 stars 0 forks source link

(Day status) Endpoint for uploading status #513

Closed sumairasaeed closed 7 years ago

sumairasaeed commented 7 years ago

This endpoint will allow to upload photos and videos as status instead of a simple text-based status message.

jekram commented 7 years ago

Currently we do not have a end point to upload photo and video?

sumairasaeed commented 7 years ago

We do have endpoints for uploading images and videos on server. I had discussed this point with @sojharo also last time if we can use same endpoints. But he insisted that it will be complicated to use same endpoint for uploading media for 'status' feature as we will have to update separate table in database for this. So he suggested to have separate endpoints for uploading and downloading image/videos for this 'status' feature.

On Mon, May 22, 2017 at 11:52 AM, Cloudkibo notifications@github.com wrote:

Currently we do not have a end point to upload photo and video?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Cloudkibo/CloudKibo/issues/513#issuecomment-303014151, or mute the thread https://github.com/notifications/unsubscribe-auth/AKbhp_MNI_NW7jZJBwY4jAhkJykwDPuZks5r8TDLgaJpZM4Nh8Os .

jekram commented 7 years ago

I want to understand the logic why we need a separate end point and not what you wanted or Sojharo wanted. As an adult sort out the issue and let me know why?

sojharo commented 7 years ago

I think there have been miscommunication. I will certainly not prefer personal choice in making a design decision so there is no point of just insisting without giving proper reasons. During my discussion with Sumaira on Friday, I gave following two main reasons to keep two features separate from each other:

  1. Modifying a single endpoint (to upload image) to support two different features will make the endpoint complex and it would mix code of two separate features.
  2. We have also plans to expose our API to external users. By mixing multiple resources in a single end point, we will make both API and its documentation complex for those users.

Sumaira might have forgotten to include these two points in the design document. My work on API doesn't just focus on fulfilling the need of our clients (mobile devices) but also focus on how the API would look to external users. Therefore, I often look at bigger picture.

Here is how people generally design the API. There are two main things in API design.

  1. Resources i.e. user, item, chat, group
  2. URLs or endpoints around these Resources.

For example, if we take resource user then we would have following endpoints around that resource.

  1. /createUser
  2. /updateUser
  3. /deleteUser
  4. /viewUser

We can have similar endpoints for other resources i.e. for item, group or chat.

e.g. for group resource, we would have endpoints

  1. /createGroup
  2. /updateGroup
  3. /deleteGroup
  4. /viewGroup

Code will be 80% same between endpoints of user and group. For example, view endpoints in both resources will have 98% same code i.e. to query the desired table and send the retrieved data to client. There is just a difference of resource name (i.e. one is user and other is group). But for sack of REST API design standards, we declare these endpoints separately.

It would be bad design practice to declare just one endpoint with flags in order to reuse the code. For example, if we create endpoint /view to view all the resources and use the flag to differentiate it would create very complex code on server side and also it would be difficult for API users. Here is how it would look like.

It would also be painful for client to handle such a endpoint. Just a small mistake of flag value, the entire response of server would be different

In our use case, the endpoints to upload images or videos in chat as attachments are for "userchat" resource. If we intend to use the same endpoint for image and video attachments for resource "day status", it would make the API complex for both us and our customers. We will have to put flags to differentiate when the image uploaded is for "userchat" resource and when it is for "day status". Both server side code and all the clients, will have to make the code complex by excessive use of flags. Using a flag in an API makes it dirty.

I prefer the reuse of code but in this case it would be at expense of REST API design principles. All resources are unique and should be treated separately. Therefore, I had thought through before asking sumaira to open the tasks for this and also told the reasons of this to sumaira. It is also very basic API concept to understand the need of separate endpoints. Following article would be a good read for all of our team to understand how we can save us from creating bad API design. I do understand these design concepts as I have spent much of time with APIs. I think all of our team members must read this once to understand what we should expect from REST API server.

https://hackernoon.com/restful-api-designing-guidelines-the-best-practices-60e1d954e7c9

sumairasaeed commented 7 years ago

Thanks @sojharo for your response. I think the confusion arose due to wrong word "insisted" used in my above comment. The word "recommended" would have been a better choice I think. Also, agreed that you mentioned reason when we discussed this. I did mention briefly, in above comment, your reason for suggesting a new endpoint that : "it will be complicated to use same endpoint for uploading media for 'status' feature as we will have to update separate table in database for this".

Thanks for elaborating the reason in more detail. It is much more clear now. Thanks for sharing useful link for API.

sojharo commented 7 years ago

Thanks @sumairasaeed. As I was working on other issues in office therefore, I couldn't read this github comment else we would have discussed this in office. :)

sojharo commented 7 years ago

I have completed the work on this and pushed the code to server.

jekram commented 7 years ago

@sojharo

sumairasaeed commented 7 years ago

There is one issue in this endpoint. Day status gets uploaded successfully, but it does not send any push notification to inform contacts that day status is being uploaded and needed to be downloaded. @sojharo kindly look into this

sumairasaeed commented 7 years ago

Debugged with @sojharo and found that there was issue in date field data type. It was fixed and now push is correctly sent. We can close this please