Skip to content

Add function to remove other authors#4196

Open
5e-Cleric wants to merge 30 commits intomasterfrom
add-remove-author-if-owner
Open

Add function to remove other authors#4196
5e-Cleric wants to merge 30 commits intomasterfrom
add-remove-author-if-owner

Conversation

@5e-Cleric
Copy link
Copy Markdown
Member

This PR introduces a check for ownership in app.js, which is prop drilled into editPage and there used to render the author list differently depending on if the user is author or owner.

UI:

image

Ownership is checked again on the server side, just in case.

@5e-Cleric
Copy link
Copy Markdown
Member Author

I don't get why are the tests failing...

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 May 12, 2025 10:27 Inactive
@dbolack-ab
Copy link
Copy Markdown
Collaborator

dbolack-ab commented May 12, 2025

The problem is

router.put('/api/:id/:author', checkClientVersion, asyncHandler(api.deleteAuthor));

I don't understand enough about the express router - it looks like it is colliding with the

router.put('/api/prune/:id/:author', checkClientVersion, asyncHandler(api.deleteAuthor));

Changing like below works:

router.put('/api/prune/:id/:author', checkClientVersion, asyncHandler(api.deleteAuthor));

@5e-Cleric
Copy link
Copy Markdown
Member Author

The problem is

router.put('/api/:id/:author', checkClientVersion, asyncHandler(api.deleteAuthor));

I don't understand enough about the express router - it looks like it is colliding with the

router.put('/api/prune/:id/:author', checkClientVersion, asyncHandler(api.deleteAuthor));

i can't find such a code in my local instance, where is that from?

@dbolack-ab
Copy link
Copy Markdown
Collaborator

dbolack-ab commented May 13, 2025

i can't find such a code in my local instance, where is that from?

/server/homebrew.api.js

510:router.put('/api/:id/:author', checkClientVersion, asyncHandler(api.deleteAuthor));

Oh, bother, I got ahead of myself on the cut and pastes and pasted the same one twice.

It was colliding with

router.put('/api/:id', checkClientVersion, asyncHandler(api.getBrew('edit', true)), asyncHandler(api.updateBrew));

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 May 13, 2025 06:23 Inactive
@5e-Cleric
Copy link
Copy Markdown
Member Author

huh, well, i fixed that, but the tests are still failing

@dbolack-ab
Copy link
Copy Markdown
Collaborator

Weird. It passes locally.

[BABEL] Note: The code generator has deoptimised the styling of /mnt/Extra/home/dbolack/Code/homebrewery-fork/node_modules/lodash/lodash.js as it exceeds the max of 500KB.
 PASS  server/homebrew.api.spec.js (9.625 s)
[BABEL] Note: The code generator has deoptimised the styling of /mnt/Extra/home/dbolack/Code/homebrewery-fork/node_modules/react-dom/cjs/react-dom.development.js as it exceeds the max of 500KB.
 PASS  tests/routes/static-pages.test.js
 PASS  tests/html/safeHTML.test.js
 PASS  server/admin.api.spec.js
 PASS  tests/markdown/mustache-syntax.test.js
 PASS  tests/markdown/variables.test.js
 PASS  tests/markdown/definition-lists.test.js
 PASS  tests/markdown/emojis.test.js
 PASS  tests/markdown/basic.test.js
 PASS  tests/markdown/paragraph-justification.test.js
 PASS  tests/markdown/non-breaking-spaces.test.js
 PASS  tests/markdown/hard-breaks.test.js
 PASS  server/middleware/content-negotiation.spec.js
------------------------------|---------|----------|---------|---------|-------------------------------------------------------------------------------
File                          | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                             
------------------------------|---------|----------|---------|---------|-------------------------------------------------------------------------------
All files                     |   60.92 |    51.41 |   52.65 |      62 |                                                                               
 client                       |    90.9 |     62.5 |     100 |     100 |                                                                               
  template.js                 |    90.9 |     62.5 |     100 |     100 | 1-5                                                                           
 client/homebrew/brewRenderer |   96.15 |    91.66 |     100 |     100 |                                                                               
  safeHTML.js                 |   96.15 |    91.66 |     100 |     100 | 8                                                                             
 client/homebrew/utils        |   28.57 |      100 |       0 |   33.33 |                                                                               
  request-middleware.js       |   28.57 |      100 |       0 |   33.33 | 6-9                                                                           
 server                       |   46.56 |       30 |   33.71 |   47.49 |                                                                               
  admin.api.js                |   61.14 |    55.26 |   41.86 |      62 | 22,33,54-58,64-72,78,83-92,98-116,121-128,133-148,153-163,304,349-350,375-381 
  app.js                      |      35 |    18.03 |    23.4 |   36.43 | ...17,422-429,434-456,461-509,516-520,530-534,573,581-588,592,606-624,628-630 
  brewDefaults.js             |     100 |      100 |     100 |     100 |                                                                               
  config.js                   |       0 |        0 |       0 |       0 |                                                                               
  forcessl.mw.js              |      40 |    33.33 |     100 |      25 | 3-6                                                                           
  googleActions.js            |    7.07 |     4.54 |       0 |    7.14 | 14-23,41-342                                                                  
  homebrew.api.js             |    68.3 |       52 |   67.56 |   70.28 | 53-86,104,133-137,338-437                                                     
  homebrew.model.js           |      20 |        0 |       0 |      20 | 8-9,36-43,47-53,57-63                                                         
  notifications.model.js      |   88.46 |       50 |     100 |   91.66 | 30,53                                                                         
  static-assets.mv.js         |    37.5 |        0 |      50 |    37.5 | 5-16                                                                          
  token.js                    |      10 |      100 |       0 |      10 | 6-22                                                                          
  vault.api.js                |      20 |        0 |       0 |   21.42 | 8-9,18-19,23-29,33,37-79,84-102                                               
 server/middleware            |   61.53 |       75 |      75 |   61.53 |                                                                               
  check-client-version.js     |       0 |        0 |       0 |       0 | 4-13                                                                          
  content-negotiation.js      |     100 |      100 |     100 |     100 |                                                                               
 shared                       |      35 |    29.54 |   33.33 |   37.14 |                                                                               
  helpers.js                  |      35 |    29.54 |   33.33 |   37.14 | 11-27,39-54,67-69,78-85,109-115,120-135                                       
 shared/naturalcrit           |   91.75 |    83.61 |   92.68 |   92.12 |                                                                               
  markdown.js                 |   91.75 |    83.61 |   92.68 |   92.12 | 124,132,150,167,336,380,439,602,696,715-716,720,842-897                       
 themes/fonts/iconFonts       |     100 |      100 |     100 |     100 |                                                                               
  diceFont.js                 |     100 |      100 |     100 |     100 |                                                                               
  elderberryInn.js            |     100 |      100 |     100 |     100 |                                                                               
  fontAwesome.js              |     100 |      100 |     100 |     100 |                                                                               
  gameIcons.js                |     100 |      100 |     100 |     100 |                                                                               
------------------------------|---------|----------|---------|---------|-------------------------------------------------------------------------------
Jest: "server/homebrew.api.js" coverage threshold for statements (70%) not met: 68.3%

Test Suites: 13 passed, 13 total
Tests:       250 passed, 250 total
Snapshots:   0 total
Time:        16.133 s

@dbolack-ab
Copy link
Copy Markdown
Collaborator

huh, well, i fixed that, but the tests are still failing

Particularly weird because looking at the test that failed, it passed.

@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented May 15, 2025

Particularly weird because looking at the test that failed, it passed.

The log results above are showing a failing result for coverage. The individual tests are passing with correct outputs, but test coverage is failing:

Jest: "server/homebrew.api.js" coverage threshold for statements (70%) not met: 68.3%

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 July 18, 2025 14:39 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 July 18, 2025 14:44 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 July 23, 2025 17:52 Inactive
@5e-Cleric 5e-Cleric added 🔍 R4 - Fixed! Awaiting re-review⭐ PR review comments addressed and removed 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging labels Jul 23, 2025
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 July 23, 2025 18:06 Inactive
@5e-Cleric 5e-Cleric moved this from Waiting for Calc's First Review to Under Review in @calculuschild's backlog Nov 10, 2025
Copy link
Copy Markdown
Collaborator

@Gazook89 Gazook89 left a comment

Choose a reason for hiding this comment

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

Left a few design related comments.

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 November 12, 2025 09:00 Inactive
@5e-Cleric
Copy link
Copy Markdown
Member Author

Last Iteration:

image

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 November 12, 2025 09:02 Inactive
Copy link
Copy Markdown
Collaborator

@Gazook89 Gazook89 left a comment

Choose a reason for hiding this comment

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

What about this:

  • crown squashed vertically just a bit and rotated with transform: rotate(-25deg) scaleY(0.6); and positioned to hang on the corner
  • set only the tag that is the current user to bold; so not the owner, but the currently logged in user if they are one of the authors. Logically I think if you are viewing the editor the current user must be in that list (or at least in the Invited Authors list).
  • remove the text-decoration on the hyperlink (use any-link selector).
  • change the title attribute on the user profile link to say "Author - Click to open [username]'s profile in a new tab" and set the link to open in a new tab.
  • and because there are now two actions in the tag (a link and a button), I really think that the 'remove' button needs some sort of hover effect...either changing the "x" color or fading in a background color of some sort for that button.
image

All that said, none of that is key to the functionality and isn't really a blocker to the PR for me.

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 November 15, 2025 19:23 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 November 19, 2025 10:00 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-4196 March 3, 2026 19:54 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature 🔍 R4 - Fixed! Awaiting re-review⭐ PR review comments addressed UI/UX User Interface, user experience

Projects

Status: Under Review

Development

Successfully merging this pull request may close these issues.

4 participants