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

[Backend] Implement Feature: Pin Notes #546

Merged

Conversation

mrfamouskk7
Copy link
Contributor

@mrfamouskk7 mrfamouskk7 commented Oct 6, 2022

Summary

Pin notes so that they will be added to priority spot, at the top for easy access, similar to Google Keep. Closes: #541

Description for the changelog

API Endpoint:

PUT /note/<NOTE_ID>/pin
Content-Type: application/json
Authorization: Bearer <TOKEN>

{
  "isPinned": true
}

Response

  1. When note gets pinned/unpinned successfully
    Response Code: 200
{
  "status": "SUCCESS",
  "message": "Note is pinned/unpinned successfully",
  "noteId": <NOTE ID>
}
  1. When note note exist in system
    Response Code: 404
{
  "status": "NOT_FOUND",
  "message": "Note not exist with ID '$noteId'"
}
  1. When unauthorized user tries to update note pin
    Response Code: 401
{
  "status": "UNAUTHORIZED",
  "message": "Access denied"
}

A picture or screenshot regarding change

NotyKT.1.mp4

Checklist

  • Build and linting is passing.
  • This change is not breaking existing flow of a system.
  • I have written test case for this change.
  • This change is tested from all aspects.
  • Implemented any new third-party library (Which not existed before this change).

dependabot bot and others added 6 commits October 6, 2022 02:05
Bumps `exposedVersion` from 0.39.2 to 0.40.1.

Updates `exposed-core` from 0.39.2 to 0.40.1
- [Release notes](https://github.com/JetBrains/Exposed/releases)
- [Changelog](https://github.com/JetBrains/Exposed/blob/master/docs/ChangeLog.md)
- [Commits](JetBrains/Exposed@0.39.2...0.40.1)

Updates `exposed-dao` from 0.39.2 to 0.40.1
- [Release notes](https://github.com/JetBrains/Exposed/releases)
- [Changelog](https://github.com/JetBrains/Exposed/blob/master/docs/ChangeLog.md)
- [Commits](JetBrains/Exposed@0.39.2...0.40.1)

Updates `exposed-jdbc` from 0.39.2 to 0.40.1
- [Release notes](https://github.com/JetBrains/Exposed/releases)
- [Changelog](https://github.com/JetBrains/Exposed/blob/master/docs/ChangeLog.md)
- [Commits](JetBrains/Exposed@0.39.2...0.40.1)

Updates `exposed-jodatime` from 0.39.2 to 0.40.1
- [Release notes](https://github.com/JetBrains/Exposed/releases)
- [Changelog](https://github.com/JetBrains/Exposed/blob/master/docs/ChangeLog.md)
- [Commits](JetBrains/Exposed@0.39.2...0.40.1)

---
updated-dependencies:
- dependency-name: org.jetbrains.exposed:exposed-core
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: org.jetbrains.exposed:exposed-dao
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: org.jetbrains.exposed:exposed-jdbc
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: org.jetbrains.exposed:exposed-jodatime
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
…le/noty-api/exposedVersion-0.40.1

[API]: Bump exposedVersion from 0.39.2 to 0.40.1 in /noty-api
@mrfamouskk7 mrfamouskk7 marked this pull request as ready for review October 6, 2022 13:53
@PatilShreyas
Copy link
Owner

PatilShreyas commented Oct 6, 2022

Thanks, @mrfamouskk7 for raising this PR. Will review it shortly. Thanks for adding sample screenshots as well. It becomes really easy to review the API surface as well. Good work 👍🏻.

@PatilShreyas PatilShreyas changed the base branch from master to feature/pin-note October 6, 2022 14:27
@mrfamouskk7
Copy link
Contributor Author

Thanks, @mrfamouskk7 for raising this PR. Will review it shortly. Thanks for adding sample screenshots as well. It becomes really easy to review the API surface as well. Good work 👍🏻.

Thanks @PatilShreyas 😄

Copy link
Owner

@PatilShreyas PatilShreyas left a comment

Choose a reason for hiding this comment

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

Did the first round of review and the change look pretty neat and clean 😃.
Just here are a few comments

@@ -97,6 +98,21 @@ class NotesController @Inject constructor(private val noteDao: NoteDao) {
}
}

fun pinNote(user: User, noteId: String, pinRequest: PinRequest): NoteResponse {
Copy link
Owner

Choose a reason for hiding this comment

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

@mrfamouskk7 Can we rename it to updateNotePin()? Because pinNote sounds like we are only pinning it and not un-pinning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense... renamed it as per suggestion.

return try {
checkNoteExistsOrThrowException(noteId)
checkOwnerOrThrowException(user.id, noteId)
val id = noteDao.pinById(noteId, pinRequest.isPinned)
Copy link
Owner

Choose a reason for hiding this comment

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

Also, DAO method can be renamed. Like, updateNotePinById or something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it as per suggestion.

response.notes shouldHaveSize 1
response.notes[0].let {
it.id shouldBe newNoteResponse.noteId
it.title shouldBe "Hey update"
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding tests 😃. Looks really nice 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 😄

@@ -21,7 +21,7 @@ buildscript {
ktlintVersion = "10.3.0"
coroutinesVersion = "1.6.4"
ktorVersion = "1.6.8"
exposedVersion = "0.39.2"
exposedVersion = "0.40.1"
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for updating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done by dependabot but sure I will take the credit 😅

@@ -33,6 +34,7 @@ interface NoteDao {
fun deleteById(id: String): Boolean
fun isNoteOwnedByUser(id: String, userId: String): Boolean
fun exists(id: String): Boolean
fun pinById(id: String, isPinned: Boolean): String
Copy link
Owner

Choose a reason for hiding this comment

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

As discussed above, please update its name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it as per suggestion.

@sandeshbodake
Copy link

Good work @mrfamouskk7 🚀

@mrfamouskk7
Copy link
Contributor Author

Good work @mrfamouskk7 rocket
Thanks @sandeshbodake 😃

Copy link
Owner

@PatilShreyas PatilShreyas left a comment

Choose a reason for hiding this comment

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

Change looks good to me @mrfamouskk7 😃. Thanks for contribution and feel free to contribute again! 🙇🏻‍♂️

@PatilShreyas PatilShreyas merged commit 0e5a39e into PatilShreyas:feature/pin-note Oct 7, 2022
@mrfamouskk7
Copy link
Contributor Author

Change looks good to me @mrfamouskk7 smiley. Thanks for contribution and feel free to contribute again! 🙇🏻‍♂️

Thanks @PatilShreyas for this opportunity 🙇‍♂️ ... I would definitely like to contribute again towards the backend side of the things as I am not much aware about Android 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Backend] Feature Request: Pin Notes
3 participants