Skip to content

Conversation

@naveen-nathan
Copy link
Contributor

@naveen-nathan naveen-nathan commented Nov 27, 2024

Jira Ticket

Jira Ticket

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Changes

  • In progressReport/app.py
    • Created the function generate_cm_from_post_parameters, which is essentially a modified version of index
    • Added validation for the post request in validate_mastery_learning_post_request
    • Load default school and class from progressReport/meta/defaults.json
  • In website/src/views/conceptMap.js
    • Added a conceptMapHTML state constant, which stores the html which renders the concept map, to be embedded in the iframe. Added code to issue a post request to CM in order to retrieve the html. Removed the old studentMastery state constant.
  • In masterymapping/index.js
    • Pasted the preexisting function getTopicsFromUser
    • Created the function getMasteryMapping to generate the dictionary used in the POST request. It takes in the dictionaries userTopicPoints, maxTopicPoints, which map node names to points earned and max points respectively, and returns the dictionary which will be issued as a POST request.
  • In api/v2/Routes/students/index.js
    • Added a route to the above functionality in masterymapping

Testing

Ran GV and confirmed that output matches data in HAID.

Checklist

  • My branch name matches the format: <ticket-id>/<brief-description-of-change>
  • My PR name matches the format: [<ticket-id>] <brief-description-of-change>
  • I have added doc-comments to all new functions (JSDoc for JS and Docstrings for Python)
  • I have reviewed all of my code
  • My code only contains major changes related to my ticket

Screenshots/Video

Screenshot 2024-12-13 at 12 35 57 PM `

Additional Notes

Post Request Schema:

The "school", "class", all concepts, and all nested "class_mastery" fields are optional. CM will default to school = "Berkeley", class = "CS10", and class_mastery = 0 in lieu of these fields being provided.

{"school": [String], "class": [String], "[Insert concept name 1]": {student_mastery: [Integer], class_mastery: [Integer]}, "[Insert concept name2]":...}

Copy link
Member

@Connor-Bernard Connor-Bernard left a comment

Choose a reason for hiding this comment

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

Couple of comments and/or changes

@Connor-Bernard
Copy link
Member

For testing, you should do a local integration test using the docker stack (you can use the dev stack)

@Connor-Bernard
Copy link
Member

Please add screenshots as necessary. Also, you can give an example using postman

@naveen-nathan
Copy link
Contributor Author

Please add screenshots as necessary. Also, you can give an example using postman

Will do!

@naveen-nathan naveen-nathan changed the title [CM-28] Process post request with mastery learning info [CM-28] Generate and process post request with mastery learning info Dec 13, 2024
setConceptMapHTML(res.data);
});
setLoading(false);
});
Copy link
Member

Choose a reason for hiding this comment

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

Why do we set the concept map html twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the selected student in the dropdown changes, the html for that student’s CM is retrieved and conceptMapHTML is updated accordingly. In the prior version of this file, this useEffect function used to update the query string when the selected student changed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so its twice for backwards compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite. This code updates conceptMapHTML every time the selected student in the dropdown (available to users with admin privileges) changes to ensure that the corresponding CM is displayed. It isn’t to ensure backwards compatibility; I meant that this useEffect function currently serves a purpose analogous to its purpose in the prior version of this file.

@TeetoToast
Copy link
Contributor

From what I see, when many nodes are collapsed, clicking on a node to expand it makes that node occupy a large amount of space. I believe that it would be better if each node expanded within an allocated space, regardless of how many nodes are open or closed. I don't know much about how that would be implemented, or if that's possible, but that's the only feedback I can think to give. Below is a ss where only the quest node is expanded, but as can be seen, it expands far beyond the view.
Screenshot 2025-02-13 at 2 30 43 PM

@naveen-nathan
Copy link
Contributor Author

naveen-nathan commented Feb 13, 2025

From what I see, when many nodes are collapsed, clicking on a node to expand it makes that node occupy a large amount of space. I believe that it would be better if each node expanded within an allocated space, regardless of how many nodes are open or closed. I don't know much about how that would be implemented, or if that's possible, but that's the only feedback I can think to give. Below is a ss where only the quest node is expanded, but as can be seen, it expands far beyond the view. Screenshot 2025-02-13 at 2 30 43 PM

Thank you for the review, Aanvi. This PR does not include changes related to node formatting; the node formatting on this branch is the same as it is on main. There is a separate PR/ticket (CM-23) dedicated to solving the issue you described, though.

Copy link
Member

@Connor-Bernard Connor-Bernard left a comment

Choose a reason for hiding this comment

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

LGTM (approved and QA by team)

@Connor-Bernard Connor-Bernard merged commit 5e5f90c into main Feb 20, 2025
4 checks passed
@Connor-Bernard Connor-Bernard deleted the CM-28/process-mastery-level-POST-request branch February 20, 2025 01:16
@naveen-nathan
Copy link
Contributor Author

LGTM (approved and QA by team)

Thanks a ton, Connor and CM team!!!

qanagattandyr pushed a commit that referenced this pull request Apr 11, 2025
…30)

* Process post request with mastery learning info

* Restore index method to original state

* Generate CM from post parameters

* Access nested student and class mastery keys

* Bug fix + return correct error code

* Fix validation bug + load constants from file

* Attempt to POST CM through the iframe + transfer CM-200 changes

* Implement api endpoint to return mastery mapping and post to CM in iframe

* Make dedicated router for mastery mapping + cleanup

* Formatting changes

* Formatting changes

* Formatting changes

* Formatting change

* Misc. changes + optimize getTopicsFromUser

* Rename variables per convention

Co-authored-by: Connor <[email protected]>

* Nest async function within useEffect

* Minor change

* Use axios to retrieve CM html

* Remove error handling

* Mark deprecated functions as such

---------

Co-authored-by: Connor <[email protected]>
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.

4 participants