-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Vendor verification #1208
Conversation
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! A lot of involved changes here but from my checks, seems very well handled. As you mention as well, this should certainly open a lot of intentional vendor related functionlity. Happy to see this get in!
I left only a minor comment inline but nothing blocking here, good stuff!
if (vendorEntry) { | ||
const [, companyName] = vendorEntry.split('|'); | ||
const vendor = await findVendorByName({ | ||
name: companyName, |
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.
May want to trim this just in case?
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 idea. I added a trim()
* @param {*} options.transaction - Sequelize transaction | ||
* @returns {Promise<*>} | ||
*/ | ||
const addUserVendor = async (userId, vendorId, { transaction }) => { |
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 handling of this (although the name puzzled me for a second, ha)! I immediately thought of the case where a vendor's company was incorrectly assigned and how it may be to have to resolve that. With this change, just updating the .txt
and having them sign in again easily handles that!
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.
Yeah I am not in love with the name but wasn't sure where best to move the term from "vendor" to "company" since a "vendor" attached to a person is their "company".
// Check if the user's company is the vendor for the associated At | ||
if (requireVendorForAt && !isAdmin) { | ||
const isVendorForAt = company && company.ats.some(at => at.id === atId); | ||
if (!isVendorForAt) { | ||
return <Navigate to="/404" replace />; | ||
} | ||
} | ||
|
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.
👍🏽 👍🏽
company: { | ||
id: '1', | ||
name: 'Company', | ||
ats: [] | ||
} |
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.
in the mock, would these need a company since the roles assigned here doesn't include 'VENDOR'?
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.
Great point! Addressed.
see #1201
This PR adds the Vendor table to the DB in order to store information about User -> Vendor and Vendor -> AT associations. With this PR, the User table has a "company" column that holds Vendors. For now, this is only used to prevent review of Candidate Test Plans unless the reviewer is either an admin or a representative of the vendor of the specified AT.
These changes could potentially enable other vendor-specific features in the future.