Skip to content

OSR commenting support

Mark Ferguson requested to merge STT-925-osr-comments into main

This is a large MR, so take your time. For my edification and improving my coding practices, I have a number of questions below that I'd like to discuss whenever possible (probably in post-stand-up).

  1. Should we have a relationship from Proposal to review (one-to-one for OSRs, one-to-many for PPRs)? Right now, it's only from review to proposal.

  2. If jwtauth permission for an endpoint doesn't allow non-tta member, is it still a good practice to check permissions in the actual rest api endpoint? For example, osr_review_by_id and osr_review_update are tta_member-only endpoints, but I also have code in the endpoint to enforce only tta members being able to update the review

  3. In the proposal_state_change service, I introduced the check for an OSR-based solicitation last sprint to determine that the state should go to In Review. In this sprint, I added create_osr_review and neither this nor the check feel right being in this location. I'd like to discuss where these should be moved to better organize and maintain these pieces of the system.

  4. In the osr_reviews endpoint my questions connected with the TODO comments:

  • should I delete the existing reviewer before creating this one? Or should there be some type of
  • create reviewer from user method?

  1. How to enforce 1 to 1 relationship between OSR Review and proposal except at application_layer?

  2. Is osr_review by_id clear enough to convey that the id is the associated proposal_id rather than review_id, or do we need to support a by_proposal_id method of sorts?

  • this impacts route naming, jwtauth permissions, orm repository methods, etc.
  • in this sprint/story, there wasn't an obvious need to be able to retrieve a specific review based on the review_id
Edited by Mark Ferguson

Merge request reports