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

New User Privacy Setting (2024) #1042

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

New User Privacy Setting (2024) #1042

wants to merge 6 commits into from

Conversation

ArabellaJi
Copy link
Contributor

@ArabellaJi ArabellaJi commented May 23, 2024

This PR is to replace the PR #937. PR 937 has too many merging conflicts, so I decide to start a new one.

UI: gordon-cs/gordon-360-ui#2031

@EjPlatzer
Copy link
Contributor

If #937 is Obsolete, can you close it @ArabellaJi?

/// <param name="userPrivacy">Faculty Staff Privacy Decisions (see UserPrivacyUpdateViewModel)</param>
/// <returns></returns>
[HttpPut]
[Route("user_privacy")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[Route("user_privacy")]
[Route("{username}/user_privacy")]

This may be personal preference, but I think it's better to always refer to a unique resource identifier rather than assuming the current authenticated user -- especially when trying to follow rest architecture.

Another option would be [Route("me/user_privacy")] but I don't know that we use that elsewhere.

Gordon360/Controllers/ProfilesController.cs Outdated Show resolved Hide resolved
Field = field;
VisibilityGroup = group;
}
public IEnumerable<string> Field { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Field an IEnumerable<string> here but just a string in UserPrivacyGetViewModel.cs?

If there is no reason and they should both be string, could these two models be merged into one?

using Gordon360.Models.CCT;
using Gordon360.Models.ViewModels.RecIM;

namespace Gordon360.Models.ViewModels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be merged with one of the other models or both of them (see comment above)


foreach (UserPrivacy_Settings row in privacy)
{
if (row.Visibility == "Private" || (row.Visibility == "FacStaff" && currentUserType != "fac"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might use an enum here instead of having string literals maintained in the code.


foreach (UserPrivacy_Settings row in privacy)
{
if (row.Visibility == "Private" || (row.Visibility == "FacStaff" && currentUserType != "fac"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum? ^^

var account = accountService.GetAccountByUsername(username);

// select all privacy settings
var privacy = context.UserPrivacy_Settings.Where(up_s => up_s.gordon_id == account.GordonID).Select(up_s => (UserPrivacyViewModel)up_s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var privacy = context.UserPrivacy_Settings.Where(up_s => up_s.gordon_id == account.GordonID).Select(up_s => (UserPrivacyViewModel)up_s);
var privacy = context.UserPrivacy_Settings.Where(up_s => up_s.gordon_id == account.GordonID).Select<UserPrivacy_Settings, UserPrivacyViewModel>(u => u);

I believe this can be cleaned up a bit to something like the following. It is clearer what is happening here IMO.

Make sure to test my syntax as I am not 100% sure.

Gordon360/Services/ProfileService.cs Outdated Show resolved Hide resolved
@jsenning
Copy link
Contributor

jsenning commented Jun 4, 2024

Arabella, The most recent changes break the UI - the drop down menus no longer have Public/FacStaff/Private in them:

sample

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.

None yet

6 participants