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

added new task and test for replace new status for locked domains #2163

Merged
merged 6 commits into from
Sep 23, 2021
Merged

added new task and test for replace new status for locked domains #2163

merged 6 commits into from
Sep 23, 2021

Conversation

OlegPhenomenon
Copy link
Contributor

No description provided.

@viezly
Copy link

viezly bot commented Sep 20, 2021 •
edited
Loading

Changes preview:

Legend:

👀 Review pull request on Viezly

@OlegPhenomenon OlegPhenomenon force-pushed the replace-upd_prohibited-to-obj_upd_prohibited-for-locked-domains branch from b2f174e to b4213c7 Compare September 20, 2021 12:42
yulgolem
yulgolem suggested changes Sep 20, 2021
View reviewed changes
app/jobs/replace_upd_to_obj_upd_prohibited_job.rb Outdated
@@ -0,0 +1,55 @@
class ReplaceUpdToObjUpdProhibitedJob < ApplicationJob
def perform(mode, rollback = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using keyword arguments when argument's count is more than one.

app/jobs/replace_upd_to_obj_upd_prohibited_job.rb Outdated
def perform(mode, rollback = false)
logger.info 'Ran ReplaceUpdToObjUpdProhibitedJob!'

start_adding_new_status_for_locked_domains(mode, rollback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

app/jobs/replace_upd_to_obj_upd_prohibited_job.rb Outdated

domain_batches.each do |domain|
if domain.locked_by_registrant?
if rollback
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to avoid nested conditions.

lib/tasks/replace_upd_to_obj_upd_prohibited.rake Outdated
desc 'Add serverObjUpdateProhibited for locked domains'
task add_new_status: :environment do
time = Benchmark.realtime do
ReplaceUpdToObjUpdProhibitedJob.perform_later('add')
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using symbols instead of strings as arguments, preferably like perform_later(action: :add) or (action: :remove)

yulgolem
yulgolem approved these changes Sep 20, 2021
View reviewed changes
app/jobs/replace_upd_to_obj_upd_prohibited_job.rb Outdated
end

def rollback_actions(action:, domain:)
if action == :add and !domain.statuses.include? 'serverUpdateProhibited'
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not tweaking execution flow here, so it's better to use && instead of and

app/jobs/replace_upd_to_obj_upd_prohibited_job.rb Outdated
logger.info "Proccesing #{count} domains of #{Domain.count}"

domain_batches.each do |domain|
if domain.locked_by_registrant? && rollback
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving this block to private method which gets domain as an argument - thus we'll lower the overall complexity

@OlegPhenomenon OlegPhenomenon force-pushed the replace-upd_prohibited-to-obj_upd_prohibited-for-locked-domains branch from 3f2bc44 to 568a142 Compare September 20, 2021 14:04
@OlegPhenomenon OlegPhenomenon marked this pull request as ready for review September 20, 2021 14:10
@keijoraamat keijoraamat removed their assignment Sep 21, 2021
@OlegPhenomenon
Copy link
Contributor Author

OlegPhenomenon commented Sep 22, 2021 •
edited
Loading

INSTRUCTIONS:

  • The task works as follows, it finds a domain that has a domain lock mark and replaces the status serverUpdateProhibited with serverObjUpdateProhibited

  • For run this task it need to type rake locked_domains:replace_new_status

  • Whole results will saved into log/replace_upd_to_obj_upd_prohibited.log

  • Therefore, it is better that the value enable_lock_domain_with_new_statuses in the application.yml file is commented out or has the status false before starting. It need to make sure before launching that these statuses mean that the domain has a domain lock, otherwise this scanner will not find the required domains. After the task has been completed, set the value enable_lock_domain_with_new_statuses to true, and then the domain with the following statuses serverDeleteProhibited, serverTransferProhibited, serverObjUpdateProhibite will be considered blocked now.

  • If for some reason it need to roll back the result, then this value enable_lock_domain_with_new_statuses must be true, and run the command rake locked_domains:rollback_replacement_new_status

@OlegPhenomenon OlegPhenomenon force-pushed the replace-upd_prohibited-to-obj_upd_prohibited-for-locked-domains branch from 84894e2 to c34e775 Compare September 23, 2021 14:00
@vohmar vohmar merged commit 9b6b7a6 into master Sep 23, 2021
@vohmar vohmar deleted the replace-upd_prohibited-to-obj_upd_prohibited-for-locked-domains branch September 28, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yulgolem yulgolem yulgolem approved these changes

Assignees

@vohmar vohmar

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OlegPhenomenon @yulgolem @vohmar @keijoraamat