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 admin-only Notes feature for courses #5650

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

Abishekcs
Copy link
Contributor

@Abishekcs Abishekcs commented Feb 13, 2024

closes #5526

What this PR does

Enable only Admins to create, edit, read and delete Course Notes

Screenshots

Before:

Before.mp4

After:

Demo.1.mp4

…ble_redux'

This commit introduces a new notes editor component. The 'toggleEditable' prop is passed from the 'editable_redux' module, enabling dynamic control over the editor's editability state.
This commit adds the 'NotesList' component responsible for rendering a list of notes, and the 'NotesRow' component for displaying individual note items within the list.
This commit adds the 'NotesPanel' component, responsible for managing a panel displaying notes for a course. It fetches and updates the list of notes and includes an edit button. The 'NotesPanelEditButton' component is introduced to handle creating new and deleting individual notes within the panel.
This commit introduces the 'NotesHandler' component, responsible for managing the display and interaction flow of course notes. It utilizes state management to handle different modal types, allowing users to view and edit course notes. The component includes integration with 'NotesPanel' and 'NotesEditor' for displaying notes and enabling note editing, respectively.
This commit adds state management functionality for course notes in the 'NotesHandler' component. The state allows for dynamic control and interaction with course notes, enhancing the overall course notes management capabilities.
This commit adds functions for fetching, saving, creating, and deleting course notes in the 'api.js' file. The new methods include 'fetchAllCourseNotes', 'fetchCourseNotesById', 'saveCourseNote', 'createCourseNote', and 'deleteCourseNote'. These functions provide the necessary API calls to interact with course notes, enabling full CRUD functionality within the application.
This commit introduces CRUD routes for Course Notes in 'routes.rb' and implements corresponding actions in 'CourseNotesController'. Additionally, the 'CourseNote' model is updated to include validations and methods for creating and updating notes. These changes collectively enable the full range of Create, Read, Update, and Delete (CRUD) operations for Course Notes within the application.
Also added comments to course_notes_action
Separated the CSS for admin only notes to a new file
From AdminQuickAaction only admin notes will be available to a admins and if the admin is staff too then all the all the AdminQuickActions are visible.
@Abishekcs Abishekcs marked this pull request as draft February 13, 2024 09:28
@Abishekcs Abishekcs marked this pull request as ready for review February 13, 2024 12:20
@Abishekcs
Copy link
Contributor Author

@ragesoss could you please review my code whenever you are free? Still working on the test code for this feature. Thanks!

@ragesoss
Copy link
Member

When I create a note, I get this error:

TypeError
Cannot read properties of undefined (reading 'id')
Call Stack
 _callee4$
  app/assets/javascripts/actions/course_notes_action.js:205:29
 tryCatch
  app/assets/javascripts/actions/course_notes_action.js:29:2404
 Generator.eval [as _invoke]
  app/assets/javascripts/actions/course_notes_action.js:29:1964
 Generator.eval [as next]
  app/assets/javascripts/actions/course_notes_action.js:29:3255
 asyncGeneratorStep
  app/assets/javascripts/actions/course_notes_action.js:31:103
 _next
  app/assets/javascripts/actions/course_notes_action.js:33:194

@ragesoss
Copy link
Member

Actually, that error happens after the server returns a 422. It looks like this is because 'edited by' is blank.

But the frontend should render an error notification if there's an error response returned by the server.

end

def create
note_details = CourseNote.new.create_new_note(course_note_params)
Copy link
Member

Choose a reason for hiding this comment

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

This is not the idiomatic Rails way to create a new record. If you do CourseNote.create(attrributes) instead, it will return a CourseNote note object which you can check for validation errors (by calling .errors on it). Or you can do CourseNote.create! and it will raise those errors directly so that you can rescue them and return an appropriately detailed error response.

@Abishekcs Abishekcs changed the title Add admin-only Notes feature for courses Add admin-only Notes feature for courses [WIP UI] Mar 19, 2024
@Abishekcs
Copy link
Contributor Author

2. Maybe instead of changing to a view of just a single note, clicking on a note could trigger an expanded accordion section to view that note within the table? Or maybe there's some other design that would feel more intuitive.

@ragesoss I am also thinking of an expanded accordion for creating new notes within the same table instead of opening a new table. Or would it be better if creating a note opens a new table entirely?

@ragesoss
Copy link
Member

  1. Maybe instead of changing to a view of just a single note, clicking on a note could trigger an expanded accordion section to view that note within the table? Or maybe there's some other design that would feel more intuitive.

@ragesoss I am also thinking of an expanded accordion for creating new notes within the same table instead of opening a new table. Or would it be better if creating a note opens a new table entirely?

I think it will be very rare for a course to have more than one or two notes, so I think keeping the same table and creating a new note within it as you propose will be a good option.

@Abishekcs
Copy link
Contributor Author

  1. Maybe instead of changing to a view of just a single note, clicking on a note could trigger an expanded accordion section to view that note within the table? Or maybe there's some other design that would feel more intuitive.

@ragesoss I am also thinking of an expanded accordion for creating new notes within the same table instead of opening a new table. Or would it be better if creating a note opens a new table entirely?

I think it will be very rare for a course to have more than one or two notes, so I think keeping the same table and creating a new note within it as you propose will be a good option.

Okay, great. I will make the changes to the UI according to this.

@Abishekcs
Copy link
Contributor Author

@ragesoss, I've made some changes to the UI. Below is a recording showcasing the updates. It's still a work in progress. If this looks okay to you, I'll proceed with this UI. For creating notes, I'll use something similar to the UI of News Feature . Please let me know if any adjustments are needed. Additionally, I think with this UI change, the number of lines of code will also reduce significantly.

2024-04-09.20-22-00.mp4

@ragesoss
Copy link
Member

ragesoss commented Apr 9, 2024

@Abishekcs yeah, I like this!

@Abishekcs
Copy link
Contributor Author

@ragesoss
I have made the UI changes for this feature according to the previous video. Also, the CSS naming convention followed here is the BEM (Block Element Modifier) convention.

I have used the convention as precisely as possible, though there might be some minor changes. However, BEM can be used as a starting point - https://cssguidelin.es/#bem-like-naming

Additionally, I have changed the table name from course_notes to admin_course_notes.

I would really appreciate if you could kindly review the code and UI. Please let me know if any further changes are required. Thank you.

Demo.1.mp4

@Abishekcs Abishekcs marked this pull request as ready for review May 6, 2024 13:28
@Abishekcs Abishekcs changed the title Add admin-only Notes feature for courses [WIP UI] Add admin-only Notes feature for courses May 6, 2024
@Abishekcs
Copy link
Contributor Author

@ragesoss any update on this PR?

@ragesoss
Copy link
Member

@Abishekcs not yet, but it's on my todo list!

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.

Add admin-only Notes feature for courses
2 participants