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.

Sign up for GitHub

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

Jump to bottom

implement domain statuses restore after domain locked, added test #1902

Merged
merged 12 commits into from
Apr 8, 2021
Merged

implement domain statuses restore after domain locked, added test #1902

merged 12 commits into from
Apr 8, 2021

Conversation

OlegPhenomenon
Copy link
Contributor

Close #1900

yulgolem
yulgolem reviewed Apr 5, 2021
View reviewed changes
app/models/concerns/domain/registry_lockable.rb Outdated
@@ -1,6 +1,8 @@
module Domain::RegistryLockable
extend ActiveSupport::Concern

# status_notes public.hstore,
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not needed here

yulgolem
yulgolem reviewed Apr 5, 2021
View reviewed changes
app/models/concerns/domain/registry_lockable.rb Outdated
@@ -9,6 +11,12 @@ def apply_registry_lock
return unless registry_lockable?
return if locked_by_registrant?

self.locked_domain_statuses_history = self.statuses.map do |status|
if LOCK_STATUSES.include? status
Copy link
Contributor

Choose a reason for hiding this comment

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

How about single-liner here?
Like status if LOCK_STATUSES.include? status

yulgolem
yulgolem reviewed Apr 5, 2021
View reviewed changes
app/models/concerns/domain/registry_lockable.rb Outdated
@@ -36,7 +44,9 @@ def remove_registry_lock

transaction do
LOCK_STATUSES.each do |domain_status|
statuses.delete(domain_status)
unless self.locked_domain_statuses_history.include? domain_status
Copy link
Contributor

Choose a reason for hiding this comment

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

and another single-liner here

@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch 2 times, most recently from 14331f9 to 80fb106 Compare April 5, 2021 13:51
@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch from 80fb106 to 9bfdb0f Compare April 5, 2021 13:54
@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch 2 times, most recently from ce49c01 to 3ff840f Compare April 6, 2021 12:39
@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch from 3ff840f to da31721 Compare April 6, 2021 12:41
@vohmar
Copy link
Contributor

vohmar commented Apr 7, 2021 •
edited
Loading

Issue

  1. set registry lock in the registrant portal
  2. remove one of the lock statuses in admin ie updateProhibited
  3. lock the domain again in the registrant portal
  4. unlock the domain in the registrant portal
    problem: only the single status set on the last registry lock reset was removed - others should be removed as well since these were also added by the locking action

@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch from 2edeb48 to 3985513 Compare April 7, 2021 09:47
@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch from 23794cd to 9c0018d Compare April 7, 2021 12:45
@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch 4 times, most recently from f468686 to aa4bb5a Compare April 8, 2021 08:50
@OlegPhenomenon OlegPhenomenon force-pushed the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch from aa4bb5a to 441a191 Compare April 8, 2021 08:59
@OlegPhenomenon OlegPhenomenon marked this pull request as ready for review April 8, 2021 13:34
@vohmar vohmar merged commit f6fcf15 into master Apr 8, 2021
yulgolem added a commit that referenced this pull request Apr 14, 2021
@yulgolem
Revert "Merge pull request #1902 from internetee/1900-removing-regist…
eac7dd3 
…ry-lock-should-not-remove-statuses-set-prior-to-setting-it"

This reverts commit f6fcf15, reversing
changes made to 26618a3.
@yulgolem
Copy link
Contributor

PR was reverted due to high amount of DB load on data migrations.
Set on hold until Sidekiq will be online in prod so we could implement the PR in question in two parts:

  1. Preparing the DB via async jobs
  2. Updating code itself.

@yulgolem yulgolem self-assigned this Apr 14, 2021
@vohmar vohmar deleted the 1900-removing-registry-lock-should-not-remove-statuses-set-prior-to-setting-it branch June 8, 2021 13:37
This was referenced Jul 8, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yulgolem yulgolem Awaiting requested review from yulgolem

Assignees

@yulgolem yulgolem

Labels
on hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Removing registry lock should not remove statuses set prior to setting it
3 participants
@OlegPhenomenon @vohmar @yulgolem