Prevent avatar 404 console errors

[imnasnainaec]

• Add property for whether there is an avatar, so that avatars aren't fetched when there isn't one.

Resolves #697

---

This change is 

[jasonleenaylor]

One more comment requested. And it feels like there needs to be a unit test in here somewhere...

Reviewed 7 of 8 files at r2.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)

---

Backend/Services/UserApiServices.cs, line 160 at r2 (raw file):

            };
            var token = tokenHandler.CreateToken(tokenDescriptor);
            Sanitize(user);

Add a comment here explaining why this method should both Sanitize before the updated AND actually return a user with a token.

[codecov-io]

Codecov Report

Merging #742 into master will decrease coverage by 3.23%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
- Coverage   50.25%   47.01%   -3.24%     
==========================================
  Files         235      198      -37     
  Lines        6159     3203    -2956     
  Branches      398      400       +2     
==========================================
- Hits         3095     1506    -1589     
+ Misses       2770     1400    -1370     
- Partials      294      297       +3     

Flag	Coverage Δ
#backend	?
#frontend	47.01% <44.44%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nents/ProjectSettings/ProjectUsers/ActiveUsers.tsx	0.00% <0.00%> (ø)
...ents/ProjectSettings/ProjectUsers/ProjectUsers.tsx	3.03% <0.00%> (ø)
src/types/user.tsx	100.00% <100.00%> (ø)
.../MergeDupGoal/MergeDupStep/MergeDupStepReducer.tsx	52.70% <0.00%> (-1.36%)	⬇️
Backend/Services/UserEditRepository.cs
Backend/Controllers/UserEditController.cs
Backend/Helper/FileUtilities.cs
Backend/Models/PasswordReset.cs
Backend/Contexts/UserEditContext.cs
Backend/Models/UserEdit.cs
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28e1f80...74b20e9. Read the comment docs.

[imnasnainaec]

Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @imnasnainaec)

---

Backend/Services/UserApiServices.cs, line 160 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Add a comment here explaining why this method should both Sanitize before the updated AND actually return a user with a token.

Comment added.

[johnthagen]

---

Backend/Models/User.cs, line 19 at r3 (raw file):

        [BsonElement("hasAvatar")]
        public Boolean HasAvatar { get; set; }

Rather than 404'ing if there is no Avatar, in typical REST API's I've used and developed, the backend could simply return JSON null for the Avatar to denote "this attribute is empty".

In OpenAPI, you can set the type of an attribute to be [string, null], which means, this can be of type string or null.

In this example, is it possible to do something like:

If the user has an avatar:

{
  "name": "Alice",
  "avatar": "http://localhost/.../avatar.jpg",
  ...
}

And if they do not:

{
  "name": "Bob",
  "avatar": null,
  ...
}

In TypeScript we can then just test what is returned from the backend

if (user.avatar !== null) {
  ...
}

This would could remove the need for a separate implicitly-related field that the client has to check.

[johnthagen]

---

Backend/Models/User.cs, line 19 at r3 (raw file):

Previously, johnthagen wrote…

Rather than 404'ing if there is no Avatar, in typical REST API's I've used and developed, the backend could simply return JSON null for the Avatar to denote "this attribute is empty".

In OpenAPI, you can set the type of an attribute to be [string, null], which means, this can be of type string or null.

In this example, is it possible to do something like:

If the user has an avatar:

{
  "name": "Alice",
  "avatar": "http://localhost/.../avatar.jpg",
  ...
}

And if they do not:

{
  "name": "Bob",
  "avatar": null,
  ...
}

In TypeScript we can then just test what is returned from the backend

if (user.avatar !== null) {
  ...
}

This would could remove the need for a separate implicitly-related field that the client has to check.

When I create a user, currently the avatar is set to "" instead of null in the database:

{"_id":{"$oid":"5f806b9eb6ac3a2c20b90014"},"avatar":"","hasAvatar":false,"name":"u","email":"a@a.com","phone":"","otherConnectionField":"","workedProjects":{},"projectRoles":{},"agreement":false,"password":"xG7RM5IP/DBMWz7fwyT7s/180IP1G13xtVcYijANdxzt5/wC3G47kb+gjnmyYrPm","username":"user","uiLang":"","token":"","isAdmin":false}

To express nullness in the type system we could do something like:

        [BsonElement("avatar")]
        public string? Avatar { get; set; }

        ...
                public User()
        {
            Id = "";
            Avatar = null;
            ...

But we'd then want to turn on nullable compiler support project-wide: https://stackoverflow.com/a/61436270

[johnthagen]

---

Backend/Models/User.cs, line 19 at r3 (raw file):

Previously, johnthagen wrote…

When I create a user, currently the avatar is set to "" instead of null in the database:

{"_id":{"$oid":"5f806b9eb6ac3a2c20b90014"},"avatar":"","hasAvatar":false,"name":"u","email":"a@a.com","phone":"","otherConnectionField":"","workedProjects":{},"projectRoles":{},"agreement":false,"password":"xG7RM5IP/DBMWz7fwyT7s/180IP1G13xtVcYijANdxzt5/wC3G47kb+gjnmyYrPm","username":"user","uiLang":"","token":"","isAdmin":false}

To express nullness in the type system we could do something like:

        [BsonElement("avatar")]
        public string? Avatar { get; set; }

        ...
                public User()
        {
            Id = "";
            Avatar = null;
            ...

But we'd then want to turn on nullable compiler support project-wide: https://stackoverflow.com/a/61436270

Opened a separate issue about tracking nullables project-wide, as that could be addressed separately from this: #754

[johnthagen]

---

Backend/Models/User.cs, line 19 at r3 (raw file):

        [BsonElement("hasAvatar")]
        public Boolean HasAvatar { get; set; }

This should be declared as

public bool HasAvatar { get; set; }

for consistency

[johnthagen]

---

Backend/Services/UserApiServices.cs, line 178 at r3 (raw file):

        {
            var users = await _userDatabase.Users.Find(_ => true).ToListAsync();
            users.ForEach(c => Sanitize(c));

This can be simplified to:

users.ForEach(Sanitize);

[johnthagen]

---

src/components/ProjectSettings/ProjectUsers/ActiveUsers.tsx, line 39 at r3 (raw file):

  private populateUsers() {
    getAllUsersInCurrentProject()
      .then(async (projUsers) => {

This can be simplified with a for..of loop:

        this.setState({ projUsers });
        const userAvatar = this.state.userAvatar;
        // for loop rather than .forEach forces each await to finish
        for (let user of projUsers) {
          if (user.hasAvatar) {
            userAvatar[user.id] = await avatarSrc(user.id);
          }
        }
        this.setState({ userAvatar });

[johnthagen]

---

src/components/ProjectSettings/ProjectUsers/ProjectUsers.tsx, line 82 at r3 (raw file):

              ),
            }));
            const userAvatar = this.state.userAvatar;

Can be simplified with for...of:

            const userAvatar = this.state.userAvatar;
            // for loop rather than .forEach forces each await to finish
            for (let user of returnedUsers) {
              if (user.hasAvatar) {
                userAvatar[user.id] = await backend.avatarSrc(user.id);
              }
            }

[johnthagen]

---

src/types/user.tsx, line 5 at r3 (raw file):

export class User {
  id: string;
  avatar: string;

Here's an example where if we encoded "missing" into the actual type system from the backend all the way to the frontend, we could express that here like:

avatar?: string;

[johnthagen]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @imnasnainaec)

[johnthagen]

---

src/types/user.tsx, line 3 at r3 (raw file):

import { Hash } from "../goals/MergeDupGoal/MergeDupStep/MergeDupsTree";

export class User {

This entire class could be simplified to:

export class User {
  id: string = "";
  avatar: string = "";
  hasAvatar: boolean = false;
  email: string = "";
  phone: string = "";
  otherConnectionField: string = "";
  projectRoles: Hash<string> = {};
  workedProjects: Hash<string> = {};
  agreement: boolean = false;
  uiLang: string = "";
  token: string = "";
  isAdmin: boolean = false;

  constructor(
    public name: string,
    public username: string,
    public password: string
  ) {}
}

This uses parameter properties to remove a large amount of duplication.

[imnasnainaec]

Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @imnasnainaec, @jasonleenaylor, and @johnthagen)

---

Backend/Models/User.cs, line 19 at r3 (raw file):

Previously, johnthagen wrote…

This should be declared as

public bool HasAvatar { get; set; }

for consistency

Done.

---

Backend/Services/UserApiServices.cs, line 178 at r3 (raw file):

Previously, johnthagen wrote…

This can be simplified to:

users.ForEach(Sanitize);

Done.

[johnthagen]

---

Backend/Models/User.cs, line 19 at r3 (raw file):

Previously, johnthagen wrote…

Opened a separate issue about tracking nullables project-wide, as that could be addressed separately from this: #754

Done.

Deferring to #754

[johnthagen]

---

src/types/user.tsx, line 5 at r3 (raw file):

Previously, johnthagen wrote…

Here's an example where if we encoded "missing" into the actual type system from the backend all the way to the frontend, we could express that here like:

avatar?: string;

Deferred to #754

[johnthagen]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @imnasnainaec)

[imnasnainaec]

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @jasonleenaylor and @johnthagen)

---

src/components/ProjectSettings/ProjectUsers/ActiveUsers.tsx, line 39 at r3 (raw file):

Previously, johnthagen wrote…

This can be simplified with a for..of loop:

        this.setState({ projUsers });
        const userAvatar = this.state.userAvatar;
        // for loop rather than .forEach forces each await to finish
        for (let user of projUsers) {
          if (user.hasAvatar) {
            userAvatar[user.id] = await avatarSrc(user.id);
          }
        }
        this.setState({ userAvatar });

Use the final method from https://lavrton.com/javascript-loops-how-to-handle-async-await-6252dd3c795/ instead.

---

src/components/ProjectSettings/ProjectUsers/ProjectUsers.tsx, line 82 at r3 (raw file):

Previously, johnthagen wrote…

Can be simplified with for...of:

            const userAvatar = this.state.userAvatar;
            // for loop rather than .forEach forces each await to finish
            for (let user of returnedUsers) {
              if (user.hasAvatar) {
                userAvatar[user.id] = await backend.avatarSrc(user.id);
              }
            }

Use the final method from https://lavrton.com/javascript-loops-how-to-handle-async-await-6252dd3c795/ instead.

---

src/types/user.tsx, line 3 at r3 (raw file):

Previously, johnthagen wrote…

This entire class could be simplified to:

export class User {
  id: string = "";
  avatar: string = "";
  hasAvatar: boolean = false;
  email: string = "";
  phone: string = "";
  otherConnectionField: string = "";
  projectRoles: Hash<string> = {};
  workedProjects: Hash<string> = {};
  agreement: boolean = false;
  uiLang: string = "";
  token: string = "";
  isAdmin: boolean = false;

  constructor(
    public name: string,
    public username: string,
    public password: string
  ) {}
}

This uses parameter properties to remove a large amount of duplication.

Done.

[johnthagen]

Reviewed 3 of 3 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved