Skip to content

Conversation

@btangmu
Copy link
Member

@btangmu btangmu commented Feb 3, 2025

-Previously only Admin could use the Generate VXML option in the ☰ menu

-New Java function canGenerateVxml, new http response item userCanGenerateVxml to encapsulate this rule on the back end

-Rename Java function isTC to isTCOrStronger, for clarity, likewise for isVetter, etc.

-Remove some superfluous conditions like userIsAdmin in (userIsAdmin || userIsTCOrStronger)

-Do not exhaustively rename ...OrStronger; in particular front end and http response json still have userIsTC, etc.; renaming for front end and json will require caution and is beyond scope of this PR

-Refactor a few lines of js for consistency and to ensure boolean: Boolean(perm?.userIsLocked) instead of (perm && perm.userIsLocked)

-Comments

CLDR-17953

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-Previously only Admin could use the Generate VXML option in the ☰ menu

-New Java function canGenerateVxml, new http response item userCanGenerateVxml to encapsulate this rule on the back end

-Rename Java function isTC to isTCOrStronger, for clarity, likewise for isVetter, etc.

-Remove some superfluous conditions like userIsAdmin in (userIsAdmin || userIsTCOrStronger)

-Do not exhaustively rename ...OrStronger; in particular front end and http response json still have userIsTC, etc.; renaming for front end and json will require caution and is beyond scope of this PR

-Refactor a few lines of js for consistency and to ensure boolean: Boolean(perm?.userIsLocked) instead of (perm && perm.userIsLocked)

-Comments
@btangmu btangmu self-assigned this Feb 3, 2025
<template>
<div v-if="!hasPermission">Please log in as Admin to use this feature.</div>
<div v-if="!hasPermission">
Please log in as a user with sufficient permissions.
Copy link
Member Author

@btangmu btangmu Feb 3, 2025

Choose a reason for hiding this comment

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

this will practically never happen, since the menu doesn't present the option if the user lacks permission

it could happen if the user saved a bookmark for this page, and then forgot to log in

}

public boolean canGenerateVxml() {
return userIsTCOrStronger(this);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only place the rule is determined

.put("userIsTC", userIsTCOrStronger(this))
// Caution: userIsVetter really means user is Vetter or Manager, but not TC or
// Admin. It should be renamed to avoid confusion.
.put("userIsVetter", userIsVetterOrStronger(this) && !userIsTCOrStronger(this))
Copy link
Member Author

Choose a reason for hiding this comment

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

tech debt; I only added a comment for now

Copy link
Member

Choose a reason for hiding this comment

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

didn't you already rename it? so the comment is out of date?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I renamed the method to userIsVetterOrStronger, but the json item is still userIsVetter

The json item needs a name like userIsVetterOrStrongerButNotTCOrStronger or userIsExactlyVetterOrManager -- such a change is beyond the scope of this ticket though


// Admin and TC users can always modify, even in closed state.
if (userIsAdmin(u) || userIsTC(u)) return null;
if (userIsTCOrStronger(u)) return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Department of Redundancy Department

if (!UserRegistry.userIsManagerOrStronger(session.user)
|| (!ORGS_MINE.equals(request.orgs)
&& !UserRegistry.userIsTC(session.user))) { // userIsTC means TC or stronger
&& !UserRegistry.userIsTCOrStronger(session.user))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

One can only add a comment like this so many times before the need to rename the function becomes irresistible

}

// The following methods were moved here from UserRegistry
// TODO: remove this todo notice
Copy link
Member Author

Choose a reason for hiding this comment

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

?!

Copy link
Member

Choose a reason for hiding this comment

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

It should have been a review comment. Apologies.

Copy link
Member Author

Choose a reason for hiding this comment

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

no problem! It was just puzzling and funny

@btangmu btangmu requested a review from srl295 February 3, 2025 19:15
Comment on lines +530 to +531
// Caution: userIsVetter really means user is Vetter or Manager, but not TC or
// Admin. It should be renamed to avoid confusion.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Caution: userIsVetter really means user is Vetter or Manager, but not TC or
// Admin. It should be renamed to avoid confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the comment should stay until the json item is renamed or otherwise refactored

@btangmu btangmu merged commit 76b711c into unicode-org:main Feb 6, 2025
14 checks passed
@btangmu btangmu deleted the t17953_b branch February 6, 2025 03:43
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.

2 participants