-
Notifications
You must be signed in to change notification settings - Fork 649
Migrate the frontend to use the new yank API #9765
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
Conversation
915c4bf
to
6b51275
Compare
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.
🔢 Self-check (PR reviewed by myself and ready for feedback.)
mirage/route-handlers/crates.js
Outdated
return notFound(); | ||
} | ||
|
||
return {}; |
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.
we should update the yank
status and add the yank_message
field here and then return the serialized version. note that there is also a test suite for the mirage route handlers to ensure that it roughly matches the real API that we are mocking here. if we implement the frontend code against an API that does not match the real API we're in for trouble 😅
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 followed the previous mock API 😅
server.delete('/api/v1/crates/:name/:version/yank', (schema, request) => {
const { name, version: versionNum } = request.params;
const crate = schema.crates.findBy({ name });
if (!crate) {
return notFound();
}
const version = schema.versions.findBy({ crateId: crate.id, num: versionNum });
if (!version) {
return notFound();
}
return {};
});
server.put('/api/v1/crates/:name/:version/unyank', (schema, request) => {
const { name, version: versionNum } = request.params;
const crate = schema.crates.findBy({ name });
if (!crate) {
return notFound();
}
const version = schema.versions.findBy({ crateId: crate.id, num: versionNum });
if (!version) {
return notFound();
}
return {};
});
Do we need to update the old one as well? Or just delete it?
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.
ouch, wow, yeah, those are definitely wrong 🙈
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.
Alright, I’ll update it as well. Initially, I thought we were focusing only on UI testing(we only checked the button name), so I didn’t consider the actual version status. I’ll make sure to update the version status correctly.
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 thought we were focusing only on UI testing(we only checked the button name), so I didn’t consider the actual version status
we have the "crates can be yanked by owner" test, but that apparently only asserts local state, which meant that the bug in the mirage implementation was hidden. and apparently we currently don't have any tests for the yank/unyank endpoints in mirage 😢
2a1466e
to
31abbd2
Compare
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.
🔢 Self-check (PR reviewed by myself and ready for feedback.)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9765 +/- ##
==========================================
- Coverage 88.80% 88.80% -0.01%
==========================================
Files 289 289
Lines 29817 29808 -9
==========================================
- Hits 26480 26470 -10
- Misses 3337 3338 +1 ☔ View full report in Codecov by Sentry. |
6741c7b
to
6670303
Compare
6670303
to
2927b7b
Compare
Signed-off-by: Rustin170506 <[email protected]>
Signed-off-by: Rustin170506 <[email protected]>
2927b7b
to
381d05c
Compare
Moved to the new API before adding frontend support for the yank message.
Tested locally:
Screen.Recording.2024-10-27.at.16.40.16.mov