-
Notifications
You must be signed in to change notification settings - Fork 14
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
Finished team page layout and photo fetching for participants #1543
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! I left a couple notes.
// const [name, setName] = useState({ | ||
// firstname: '', | ||
// lastname: '', | ||
// }); |
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 think the user hook has a fullName field that we could use for a clean first/last name. I did a little bit of digging and we can't really get the gordon id very easily from the hook so we might need to add an AD_Username to a Participant viewmodel.
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 note for the future!
<Grid item> | ||
<img src={''} alt="Team Icon" width="85em"></img> | ||
</Grid> | ||
|
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 if this is a temporary or permanent fix to the spacing issue? I'll assume it's temporary.
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.
This is a Cam question, as it was in his code for the activity/home page layout that I tried to mimic.
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.
Haha yeah temporary, we can change it at some point.
I am going to leave branch here for cam to review. |
"Pass user id probably better than AD username, but this requires us have dummy data and a complete participant fetching protocol as we need more data than just user ID" Hey @silasw2, I don't know if this is what you are suggesting, but we avoid using people's Gordon IDs anywhere we can in 360. Our end goal is that the only places we use IDs are where explicitly necessary such as showing the user their ID # on their profile. Since we have not followed the practice of creating a separate user ID just for 360, we use the account AD_Username as their identifier. |
@bennettforkner if this is the case, would it be better to start identifying people in our Participant Table (back end) by AD_Username? or in the backend we should still be using Jenzabar IDs |
The answer is yes. If we were completely redesigning 360 or if it was worth it to do a complete overhaul of the backend, we would really just create our own unique keys (rather than using a string username) that we would use as identifiers everywhere. Unfortunately, that will likely never get the attention it would need as we do not have the bandwidth to address it. Therefore, we settle for what we can fix now, which is primarily using ad_username (or just "username") as the identifier between the UI and API. See API #578 for the raised issue about it. |
Got it, I'll start re-factoring our schema and API accordingly then. |
And I'm realizing that you were just talking about your participation table for RecIM. I would guess we want to keep it consistent, but I wonder what @EjPlatzer thinks. |
I would wait for Evan to confirm. I'm not 100% sure on your schema specifically. @EjPlatzer, should they use GordonIDs, AD_Username, or a new key as their unique identifier for users in the recim tables? |
This is what I thought, but Amos had mentioned using ID as an identifier, but he might have made participant ID so maybe I misunderstood. |
Great work here @silasw2! Good call on the MUIList thing; we should switch the listings to use that for sure! And I think we should probably just be using student usernames. The other option would be our own ParticipantID but that would probably just add extra fetches for no reason. Though if we're thinking about the possibility of expanding the product, ParticipantID would allow us to have that in-house, with the Participants table linking ParticipantID with username (or whatever the college uses). |
I'll add my two cents to the conversation even though this has been closed. I think Bennett was trying to clarify that we want to keep ID Numbers out of the Front End as much as possible - which means not using them to communicate with the API, in addition to stripping them from database objects before sending them to the UI, and not displaying them anywhere they aren't strictly necessary in the UI. However, Gordon ID is still the preferred user identifier in the database. Luckily, the fix is pretty easy - use AD Username as an identifier in the UI and when communicating with the API. In the API, it is fairly trivial to accept AD Usernames and then get the ID from the username so that you can use the ID to communicate with the database. TLDR: Avoid using/having Gordon ID in the UI. Use AD Username to identify users. In the API, accept AD Username but use Gordon ID. |
This is what the current team page looks like (and subsequently what the participant listing looks like).
Somethings to consider in this PR: