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

Add name management functionality for Characters and Pets #5

Closed
wants to merge 3 commits into from

Conversation

Lego6245
Copy link

@Lego6245 Lego6245 commented Dec 6, 2021

The following PR adds the approve_names route, which pulls all names that are still pending, and allows a GM user to either approve or reject names, updating them in the DB.

  • Adds approve_names template for viewing pending character and pet names
  • Adds functionality to approve or reject names from the interface
  • Adds approve_names get/post routes with appropriate functionality
  • Moves the navbar into its own template to allow for less code duplication

Open question: Is this the correct way to do name approval? Does anything else need to happen for it to work correctly?

Fixes #2
Fixes #8

@Lego6245 Lego6245 changed the title Add name management functionality Add name management functionality for Characters and Pets Dec 7, 2021
<table class="table table-striped">
<tr>
<th>Pet ID</th>
<th>Current Name</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "Custom Name" (or just "Name") to match the column header for character names?

else:
message = 'Pet name {} approved'.format(pending_name)
else:
query = "UPDATE pet_names SET approved = 0 WHERE id = %s"
Copy link
Contributor

Choose a reason for hiding this comment

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

The server itself does not seem to support rejecting pet names yet. Unless I am missing something it only supports pending approval (1) and approved (2) at the moment, see relevant code.

Would it make sense to omit rejecting pet names for now, until that has been implemented server-side?
Otherwise it might cause issues in the future when it is implemented for the server but they chose a different implementation, other than using value 0 to represent renaming required.

Choose a reason for hiding this comment

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

The server itself does not seem to support rejecting pet names yet. Unless I am missing something it only supports pending approval (1) and approved (2) at the moment, see relevant code.

Would it make sense to omit rejecting pet names for now, until that has been implemented server-side? Otherwise it might cause issues in the future when it is implemented for the server but they chose a different implementation, other than using value 0 to represent renaming required.

The server does support rejecting names. Changing the approved value to 0 rejects the name, and forces the user to rename the pet when they bring it out next.

However, that being said, it does NOT support forcing a user to rename a pet that's already been approved. To accomplish this, you need to set a 0 in the pet_names table AND set a 0 on the m tag in the respective pet tag in the user's xml data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarification!

However, that being said, it does NOT support forcing a user to rename a pet that's already been approved.

This might not be an issue (for now) because if I understand these changes correctly, only unapproved pet names are displayed. So unless you are manually editing the database to change an already approved name back to unapproved, this situation should not occur.

(Though @Lego6245 can probably answer that better.)

Copy link
Author

Choose a reason for hiding this comment

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

Correct on all counts.

While there is no reference in server code, in client code, it handles the state of '0' correctly.

Only pending names are shown. There is no way to re-force a name to be approved (currently).


{% if names %}
<h3>Character Names</h3>
<table class="table table-striped">
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about including some default text like "No pending approvals" when there are neither pending character name nor pet name approvals?
This would make it a little bit more obvious why the page is empty.

Though I am not a member of this project, so feel free to just ignore that suggestion.

@aronwk-aaron
Copy link
Member

Nexus Dashboard has this functionality and is tested, Do you will want to get this in the app or just use the new app?

@aronwk-aaron
Copy link
Member

See Nexus Dashboard for this functionality and more

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.

Allow approval of pet names Allow for management of custom names
5 participants