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

Finished team page layout and photo fetching for participants #1543

Merged
merged 2 commits into from
Oct 16, 2022

Conversation

silasw2
Copy link
Contributor

@silasw2 silasw2 commented Oct 15, 2022

This is what the current team page looks like (and subsequently what the participant listing looks like).

image

Somethings to consider in this PR:

  • Maybe switch from the grid div system we have been using for listings and switch to MUI List as it would be more synonymous with the rest of 360
  • 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

@silasw2 silasw2 requested review from cpabbot and amos-cha and removed request for cpabbot October 15, 2022 17:56
Copy link
Contributor

@amos-cha amos-cha left a 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.

Comment on lines +40 to +43
// const [name, setName] = useState({
// firstname: '',
// lastname: '',
// });
Copy link
Contributor

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.

Copy link
Contributor Author

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>
&nbsp;&nbsp;&nbsp;&nbsp;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@silasw2
Copy link
Contributor Author

silasw2 commented Oct 15, 2022

I am going to leave branch here for cam to review.

@bennettforkner
Copy link
Contributor

"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.

@amos-cha
Copy link
Contributor

amos-cha commented Oct 15, 2022

"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

@bennettforkner
Copy link
Contributor

bennettforkner commented Oct 15, 2022

@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.

@amos-cha
Copy link
Contributor

@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.

@bennettforkner
Copy link
Contributor

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.

@bennettforkner
Copy link
Contributor

bennettforkner commented Oct 15, 2022

Got it, I'll start re-factoring our schema and API accordingly then.

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?

@silasw2
Copy link
Contributor Author

silasw2 commented Oct 15, 2022

"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.

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.

@silasw2 silasw2 merged commit be44cf7 into recim Oct 16, 2022
@cpabbot
Copy link
Contributor

cpabbot commented Oct 16, 2022

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).

@EjPlatzer
Copy link
Contributor

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.

@cpabbot cpabbot deleted the add-team-page branch October 17, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants