-
Notifications
You must be signed in to change notification settings - Fork 33
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
Y24-166: Add new endpoint on V2 API for creating and querying Asset Audits #4181
Conversation
We should write some tests for this... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions
Another thing I thought of, by the way. The originating issue for this work says that the endpoints for Sequencescape V2 API should be documented. I don't know if we have existing documents for the API. If so, we need to add this new one to the documentation. If we don't have documentation, we probably need to look at how to auto-generate them. |
Good point. I suspect that documentation is limited (it's certainly not easily findable). I think this might need to be spun out into a new ticket. |
I stand corrected, it is literally the first thing in the readme. However, when I run them locally, it errors out with I think verify and update Sequencescape API docs, might be a useful story. |
Co-authored-by: Stuart McHattie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good. I've added a couple of comments
# | ||
# @param _context [JSONAPI::Resource::Context] not used. | ||
# @return [Array<Symbol>] the list of updatable fields. | ||
def updatable_fields(_context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're using the wrong definition for this method: https://jsonapi-resources.com/v0.9/guide/resources.html#Creatable-and-Updatable-Attributes
It would probably work if we use def self.updatable_fields(_context)
instead of making it an instance method. If it does work, we need to replicate that across other resources. If it doesn't, we probably need to remove this method as it's not doing what the comment says it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks! The correct method naming works - I've updated the code and tests in 3528e78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fix other classes that don't include self.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Volume Update one (DONE), or in SS in general? If the latter, I fear some functionality might break unless it's tested well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SS in general. I agree and it probably needs to be a separate cleanup task. The intention is there in other models, but it won't be doing what the developer intended. We either need to remove the definition or accept it might break some use cases (by trying to correct the code). See, for example: https://github.com/sanger/sequencescape/blob/develop/app/resources/api/v2/plate_purpose_resource.rb#L80-L82
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purposes are not supposed to be updated using the API. It is a dangerous thing to do. We edit the DB for changing if we have to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would prefer the Limber config for a purpose be the source of truth so config:generate should update the database for purposes. Had several issues in the past where flags set previously on purposes were not updated as a pipeline got developed and then broke on deployment as we forget to check and manually update the database. But agree dangerous to allow generally, would need to restrict it to only config:generate on deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's going to be tricky. The only things I can think of are:
- Make the fields updatable and accept that anyone with access to the API could update them.
- Add a different endpoint that Limber uses to make these changes -- not great as it still leaves an open endpoint for such changes.
- Have some way of changing the return value of this method under some circumstances:
- Perhaps the
config:generate
could use a different API key and perhaps that key could be identified in this method. Kind of like a super-user flag on the API key config. (I've not looked into this) - Change Sequencescape to a temporary mode using a feature flag which allows all fields to be updated. I was told
flipper
has a UI -- never seen it, but perhaps that could be used? - Something even more exotic, like allowing changes at a certain time of day, allowing it for 5 minutes after SS was last deployed or allowing the change if the request was made three times in a short period. I don't like these options.
- Perhaps the
I think using a different API key is probably the most appropriate option to balance convenience and security, but it's not the simplest to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation for JSONAPI:Resources you can implement something that checks the state of the execution and therefore you could attach the behaviour to the request in some way. I'm still unsure whether you can see the headers of the request from inside the resource, but it looks like you can pass that from the controller using the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the different API key idea too, deployments could use it.
Leave it as you've got it for now and we can discuss it in a retro or something.
let(:resource_model) { create(:asset_audit) } | ||
let(:updated_resource_model) do | ||
resource_model.update(witnessed_by: 'new_witness_user') | ||
resource_model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the same applies here as I mentioned in the other PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see 5dad22b for the fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good I think.
Left a comment querying the list of fields that should be creatable. Wasn't clear why some are and some aren't.
Code Climate has analyzed commit 26a9aed and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 86.7% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Closes #4191
Changes proposed in this pull request
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code