-
Notifications
You must be signed in to change notification settings - Fork 326
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 lock routes #3421
base: master
Are you sure you want to change the base?
Add lock routes #3421
Conversation
Still working on functionality, visibility will come next. |
I'm afraid to report the number of locks doesn't seem to work for me. Also, is lock review request supposed to work yet? Lock removal works fine, and i could set another lock, but doesn't seem to block in share page any longer. |
It should work - the change to I've updated to the latest master, I'll have a look soon. |
Looks like it's getting back 503 Service Unavailable errors, possibly related to DB request timeouts. |
Have in mind the main brew count function in admin returns always a 503, you might be reading that one as well. Better to disable that automatic query on page load for the moment. |
The addition of indexes on the database have greatly increased the speed of the lock functions. |
Well, with that, the lock count works, and i can see lock review requests. |
|
const version = require('../../../package.json').version; | ||
|
||
const addHeader = (request)=>request.set('Homebrewery-Version', global.version); | ||
const addHeader = (request)=>request.set('Homebrewery-Version', version); |
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.
Not sure the purpose of this change. global.version
works.
router.get('/api/lock/count', mw.adminOnly, async (req, res)=>{ | ||
try { | ||
const countLocksQuery = { | ||
lock : { $exists: true } | ||
}; | ||
const count = await HomebrewModel.countDocuments(countLocksQuery) | ||
.then((result)=>{ | ||
return result; | ||
}); | ||
return res.json({ | ||
count | ||
}); | ||
} catch (error) { | ||
console.error(error); | ||
return res.json({ status: 'ERROR', detail: 'Unable to get lock count', error }); | ||
} | ||
}); |
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.
Might I suggest the following, and a similar pattern to the other try/catch blocks?
router.get('/api/lock/count', mw.adminOnly, async (req, res)=>{ | |
try { | |
const countLocksQuery = { | |
lock : { $exists: true } | |
}; | |
const count = await HomebrewModel.countDocuments(countLocksQuery) | |
.then((result)=>{ | |
return result; | |
}); | |
return res.json({ | |
count | |
}); | |
} catch (error) { | |
console.error(error); | |
return res.json({ status: 'ERROR', detail: 'Unable to get lock count', error }); | |
} | |
}); | |
router.get('/api/lock/count', mw.adminOnly, async (req, res)=>{ | |
const countLocksQuery = { lock : { $exists: true } }; | |
const count = await HomebrewModel.countDocuments(countLocksQuery); | |
.catch((error)=>{ | |
console.error(error); | |
return res.json({ status: 'ERROR', detail: 'Unable to get lock count', error }); | |
}); | |
if (count) | |
return res.json({count}); | |
}); |
return res.json({ status: 'LOCKED', detail: `Lock applied to brew ID ${brew.shareId} - ${brew.title}`, ...lock }); | ||
} catch (error) { | ||
console.error(error); | ||
return res.json({ status: 'ERROR', error, message: `Unable to set lock on brew ${req.params.id}` }); |
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 utilize our central error handling middleware. I.e., just throw
the error here, and the error handler in app.js
will automatically organize the error into a common format (include a stack trace, etc.), console.error it, and send it to the client.
This PR contributes towards Issue #3326.
This PR adds the following routes to the Admin API:
/admin/lock
:GET
returns the number of documents in the collection that have thelock.locked
property set totrue
./admin/lock/:id
:POST
takes a brew ID and a JSONlock
object; updates the identified brew with the properties of thelock
object./admin/unlock/:id
:PUT
takes a brew ID; removes the lock from the identified brew/admin/lock/reviews
:GET
returns a collection of documents that has thelock.locked
set totrue
andlock.reviewRequested
exists./admin/lock/review/request/:id
:PUT
updates the identified brew'slock
object with areviewRequested
property (current date/time). NON-ADMIN ROUTE/admin/lock/review/remove/:id
:PUT
updates the identified brew'slock
object to remove thereviewRequested
property without removing the entirelock
.