Update Collection View

[raclim]

Fixes #3269

Changes:

• passes username as a prop to Collection component, which was previously not accessible
• updates conditions to determine isOwner, which was previously returning undefined instead of a boolean
• imports removeFromCollection action directly into CollectionItemRow

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[release-com]

Release Environments

This Environment is provided by Release, learn more!
To see the status of the Environment click on Environment Status below.

• p5.js-web-editor
  • app-ted9e7a.release.editor.p5js.org/
  • preview-ted9e7a.release.editor.p5js.org/

🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-5a8e51661b

[lindapaiste]

There is definitely room to clean this up further.

The user != null check is unnecessary because user is state.user which is always an object even when there is no one logged in.

You are now passing username as a prop, which is the username of the collection owner, so you can use that instead of collection?.owner?.username. Using username has a few advantages, including that it won't be undefined while the collection is fetching so you get an accurate boolean during loading.

Additionally, username is never undefined. So there is no possibility that username === user.username when there is no logged in user (and user.username is undefined). So you don't need to explicitly check that there is a user.username. If username === user.username is true then there is a user logged in who is the the owner of this collection. If it is false then either there is no user logged in or the logged in user is not the owner.

So you can write const isOwner = () => username === user.username and it will have the same effect.

When I converted this component I wrote it as:

const currentUsername = useSelector((state) => state.user.username);
const isOwner = username === currentUsername;

[lindapaiste]

IMO it's important to use descriptive variable names like currentUsername and ownerUsername to avoid bugs like the one that you fixed here.

I know that I can be rude sometimes but I think you can understand my frustrations. Because I already converted this component over a year ago and it was cleanly written and did not have that sort of mistake.

[raclim]

Thanks for catching these, there's probably still a lot of other areas this component could be improved here!

Since I wasn't able to get through all of your work and made some changes to these files that differed from yours, I do get your frustrations on how it probably feels like this could've been preventable. Although it'll be a lot of work since I didn't keep up with the PRs from back then, if you'd like to, we can get still try to get them in. I could be wrong, but I think most of them could probably be merged with some minimal changes except for a few (like the Common Table Components and the CodeMirror one, esp since it's part of the fellowship). I was also planning to upgrade the Node.js minor version in the near future, which I think might conflict with this one.

This is a bit of a tangent and definitely depends on the context, but I also personally feel that it's okay for code to end up a little messy and have mistakes sometimes! While I do think it's important to try not to make them, I don't want to view them negatively because I feel like they're a really normal part of any process, and in most cases, can usually be fixed.

[raclim]

Wanted to add that I meant the latter more as a point of conversation, rather than disagreeing!