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 suspend / unsuspend to the console #2675

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

ptkach
Copy link
Collaborator

@ptkach ptkach commented Feb 13, 2025

This change is Reviewable

@ptkach ptkach requested a review from gbrodman February 13, 2025 18:36
@ptkach ptkach force-pushed the consoleSuspendDomain branch from 913ba3d to d6fae10 Compare February 13, 2025 19:26
Copy link
Collaborator

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ptkach)


a discussion (no related file):
not sure what's up with the test failures, that's odd


core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java line 65 at r1 (raw file):

  private static final Gson GSON = GsonUtils.provideGson();

  private static ImmutableSet<StatusValue> serverSuspesionStatuses =

nit: "serverSuspensionStatuses"


core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java line 46 at r1 (raw file):

 * Console endpoint to perform the same action to a list of domains.
 *
 * <p>All requests must include the {@link ConsoleDomeeeainActionType.BulkAction} to perform as well

eee


console-webapp/src/app/domains/domainList.component.html line 36 at r2 (raw file):

          <span>Registry Lock</span>
        </button>
        <button

Is there any way to have these only enabled if the domain is already in the proper state?

Meaning, we probably don't want the "unsuspend" button to be enabled if the domain isn't suspended.


console-webapp/src/app/domains/domainList.component.ts line 69 at r1 (raw file):

}

enum Operation {

is it necessary to have the second bit of each of these, or can the enum just be {deleting, suspending, unsuspending}?


console-webapp/src/app/domains/domainList.component.ts line 341 at r2 (raw file):

        reason,
        this.registrarService.registrarId(),
        BULK_ACTION_NAMES.SUSPEND

Don't we need separation between SUSPEND and UNSUSPEND here?

@ptkach ptkach force-pushed the consoleSuspendDomain branch from d6fae10 to d8a4a07 Compare February 13, 2025 21:07
Copy link
Collaborator Author

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gbrodman)


a discussion (no related file):

Previously, gbrodman wrote…

not sure what's up with the test failures, that's odd

It seems to have been related to eee typo in one of the javadoc comments


console-webapp/src/app/domains/domainList.component.html line 36 at r2 (raw file):

Previously, gbrodman wrote…

Is there any way to have these only enabled if the domain is already in the proper state?

Meaning, we probably don't want the "unsuspend" button to be enabled if the domain isn't suspended.

I've thought about it too and for now I've figured to keep it at the server side like, but I can change. It seems like we can enable / disable (un)suspend buttons if all statuses available or missing. Meaning if 5 statuses present - enable unsuspend, if all 5 statuses missing - enable suspend. Does it make sense?


console-webapp/src/app/domains/domainList.component.ts line 69 at r1 (raw file):

Previously, gbrodman wrote…

is it necessary to have the second bit of each of these, or can the enum just be {deleting, suspending, unsuspending}?

Yeah in runtime it's just an object, so without duplication it just gives - 1, 2, 3.... Google suggested getting a keyname by accessing it via Operation[Operation.deleting] but it didn't work for me so I resorted to this option.


console-webapp/src/app/domains/domainList.component.ts line 341 at r2 (raw file):

Previously, gbrodman wrote…

Don't we need separation between SUSPEND and UNSUSPEND here?

Yep, found it during testing


core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java line 46 at r1 (raw file):

Previously, gbrodman wrote…

eee

mystery


core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java line 65 at r1 (raw file):

Previously, gbrodman wrote…

nit: "serverSuspensionStatuses"

Yep

Copy link
Collaborator

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @ptkach)


console-webapp/src/app/domains/domainList.component.html line 36 at r2 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

I've thought about it too and for now I've figured to keep it at the server side like, but I can change. It seems like we can enable / disable (un)suspend buttons if all statuses available or missing. Meaning if 5 statuses present - enable unsuspend, if all 5 statuses missing - enable suspend. Does it make sense?

I think maybe we enable suspension if any one of the statuses is missing -- that way it could fill the remaining statuses (and wouldn't prevent us from getting in some weird/bad state).

Correspondingly, we'd only enable un-suspend if every one of the five statuses is there. Thoughts?


console-webapp/src/app/domains/domainList.component.ts line 341 at r2 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Yep, found it during testing

might be better to just pass in the BULK_ACTION_NAME enum directly rather than using a separate boolean? seems clearer I think

also this is nitty but we probably want the enum title to be BULK_ACTION_NAME because names tend to be singular

Copy link
Collaborator

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

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

@ptkach ptkach force-pushed the consoleSuspendDomain branch from d8a4a07 to ba4a681 Compare February 14, 2025 16:11
Copy link
Collaborator

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ptkach)


console-webapp/src/app/domains/domainList.component.ts line 287 at r4 (raw file):

  }

  isDomainUnsuspendable(domain: Domain) {

nice

@ptkach ptkach force-pushed the consoleSuspendDomain branch from ba4a681 to 26eaaf4 Compare February 14, 2025 19:53
@ptkach ptkach enabled auto-merge February 14, 2025 20:00
@ptkach
Copy link
Collaborator Author

ptkach commented Feb 14, 2025

Tested in alpha - works as expected

@gbrodman
Copy link
Collaborator

Tested in alpha - works as expected

wooooo

@ptkach ptkach added this pull request to the merge queue Feb 14, 2025
Merged via the queue into google:master with commit 95831bc Feb 14, 2025
9 checks passed
@ptkach ptkach deleted the consoleSuspendDomain branch February 14, 2025 21:28
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