Skip to content
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

Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open

Conversation

G-Ambatte
Copy link
Collaborator

@G-Ambatte G-Ambatte commented Apr 21, 2024

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 the lock.locked property set to true.
  • /admin/lock/:id : POST takes a brew ID and a JSON lock object; updates the identified brew with the properties of the lock 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 the lock.locked set to true and lock.reviewRequested exists.
  • /admin/lock/review/request/:id : PUT updates the identified brew's lock object with a reviewRequested property (current date/time). NON-ADMIN ROUTE
  • /admin/lock/review/remove/:id : PUT updates the identified brew's lock object to remove the reviewRequested property without removing the entire lock.

@G-Ambatte G-Ambatte mentioned this pull request Apr 21, 2024
4 tasks
@G-Ambatte G-Ambatte self-assigned this Apr 21, 2024
@G-Ambatte G-Ambatte added the sub-epic Sub-task of an Epic label Apr 21, 2024
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3421 May 11, 2024 11:59 Inactive
@5e-Cleric
Copy link
Member

5e-Cleric commented May 11, 2024

We could use two columns in the lock tab there

Attaching screenshot for reference

image

@G-Ambatte
Copy link
Collaborator Author

Still working on functionality, visibility will come next.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3421 June 28, 2024 22:35 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3421 June 29, 2024 03:29 Inactive
@5e-Cleric
Copy link
Member

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.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3421 July 22, 2024 22:29 Inactive
@G-Ambatte
Copy link
Collaborator Author

It should work - the change to homebrew.api:getBrew should throw an error on any locked brew, the only exception being a valid edit attempt, in which case the lock notification prompt should fire.

I've updated to the latest master, I'll have a look soon.

@G-Ambatte
Copy link
Collaborator Author

G-Ambatte commented Jul 22, 2024

Looks like it's getting back 503 Service Unavailable errors, possibly related to DB request timeouts.
Might be a case of adding an index, or even pushing locks to it's own collection, rather than adding to the homebrews collection.

@5e-Cleric
Copy link
Member

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.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3421 July 25, 2024 10:11 Inactive
@G-Ambatte
Copy link
Collaborator Author

The addition of indexes on the database have greatly increased the speed of the lock functions.

@5e-Cleric
Copy link
Member

Well, with that, the lock count works, and i can see lock review requests.

@G-Ambatte G-Ambatte added blocked Waiting on a dependency, other feature, etc., first and removed blocked Waiting on a dependency, other feature, etc., first labels Aug 2, 2024
@5e-Cleric
Copy link
Member

5e-Cleric commented Sep 2, 2024

  • /admin/lock : GET returns the number of documents in the collection that have the lock.locked property set to true.
  • /admin/lock/:id : POST takes a brew ID and a JSON lock object; updates the identified brew with the properties of the lock 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 the lock.locked set to true and lock.reviewRequested exists.
  • /admin/lock/review/request/:id : PUT updates the identified brew's lock object with a reviewRequested property (current date/time). NON-ADMIN ROUTE
  • /admin/lock/review/remove/:id : PUT updates the identified brew's lock object to remove the reviewRequested property without removing the entire lock.

Comment on lines +2 to +4
const version = require('../../../package.json').version;

const addHeader = (request)=>request.set('Homebrewery-Version', global.version);
const addHeader = (request)=>request.set('Homebrewery-Version', version);
Copy link
Member

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.

Comment on lines +142 to +158
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 });
}
});
Copy link
Member

@calculuschild calculuschild Sep 24, 2024

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?

Suggested change
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}` });
Copy link
Member

@calculuschild calculuschild Sep 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sub-epic Sub-task of an Epic
Projects
Status: Waiting for Calc's First Review
Development

Successfully merging this pull request may close these issues.

4 participants