Organization Notes and Logs #1188
Conversation
There was a problem hiding this comment.
To fully close #998, client/src/pages/lab_management/audit_logs/AuditLogEntity.tsx needs to be modified such that a handler for organizations is present in the getEntityUrl function
commit suggestion from ZZwatermelon. Required notes Co-authored-by: ZZwatermelon <63483612+ZZwatermelon@users.noreply.github.com>
commit suggestion from ZZwatermelon. non-null notes Co-authored-by: ZZwatermelon <63483612+ZZwatermelon@users.noreply.github.com>
| // If the entity is an organization, use its data to build its URL | ||
| if (entityType === "organization") { | ||
| const { data: orgData } = useQuery(GET_ORG_BY_ID, { | ||
| variables: { id: id }, | ||
| }); | ||
| url = `/makerspace/${makerspaceID}/currency?a=${orgData?.getOrganizationByID?.username}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not needed, you should be able to make the URL the same way as all the others in getEntityUrl.
When we create the log message in the resolver, we give 3 pieces of information:
- The ID
- The type ("organization" in this case)
- The displayname
It is then kept in the log message with the format {id:type:displayname}.
This component simply parses that information out of that format to construct a link and have it display with the given displayname.
It extracts that information on line 45, so there is no need to make any calls to the backend.
There was a problem hiding this comment.
What should I do if /makerspace/${makerspaceID}/currency?a=${displayname} brings up more than one organization?
There was a problem hiding this comment.
That is fine, it is okay to assume that even if it does not limit it to one, it will at least reduce the options to the point that the user will be able to determine for themselves what account they are looking for.
That being said the ?a={} should take an ID in ?a={id} which should limit it to one. getEntityUrl shouldn't have the orgUsername parameter added to the function.
I will make another comment about it, I thought the OrganizationsRepo search function already did that but I checked and it does not. It should be done in a way similar to the CurrencyAccountsRepo does it.
| export const GET_ORG_BY_ID = gql` | ||
| query GetOrganizationByID($id: ID!) { | ||
| getOrganizationByID(id: $id) { | ||
| id | ||
| username | ||
| notes | ||
| displayname | ||
| accountID | ||
| account { | ||
| id | ||
| balance | ||
| } | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
Even though you shouldn't need this to construct the audit log entity, this is fine to leave in case someone needs to use it for something in the future.
Also good job separating out the queries into their own file!
|
|
||
| return await knex("Organizations").select("*").limit(limit) | ||
| .whereILike("displayname", `%${searchText}%`) | ||
| .orWhereILike("username", `%${searchText}%`); |
There was a problem hiding this comment.
| .orWhereILike("username", `%${searchText}%`) | |
| .orWhere("id", Number.isNaN(Number(searchtext)) ? -1 : searchText); |
What I meant by it should handle passing in the id.
There was a problem hiding this comment.
I put just this line in before the other conditions, and I got a currency account (that belonged to a user) whose ID matched the search text. For example, the launchinitiative org has its ID in the Organizations table as 2. With ?a=2, I get an account whose ID in the CurrencyAccounts table (and therefore Account ID) is 2.
Should I be trying to get the organization's accountID instead? Or is there another way to handle this?
There was a problem hiding this comment.
Sorry, I just noticed this.
In getEntityUrl it currently constructs the organization URL like this:
return `/makerspace/${makerspaceID}/currency?a=${orgUsername}`;
the /currency is going to the currency/accounts page when we want to go the organizations page, which would look like this:
return `/makerspace/${makerspaceID}/organizations?q=${orgID}`;
This causes another problem because you will probably get a lot of results. I think an easy way around that is where if the search query is a number return only results where the orgID is that number, while if it is NaN, do the search it is currently doing now.
Does that make sense?
There was a problem hiding this comment.
Ok, I'll try implementing this.
ZZwatermelon
left a comment
ZZwatermelon
left a comment
There was a problem hiding this comment.
Everything else looks good 👍
| } | ||
|
|
||
| function getEntityUrl(entityType: string, id: string, makerspaceID: string) { | ||
| function getEntityUrl(entityType: string, id: string, makerspaceID: string, orgUsername?: string) { |
There was a problem hiding this comment.
Don't need orgUsername anymore
Removed optional parameter 'orgUsername' from getEntityUrl function.
Closes: #997
Closes: #998