Worker/estimate default language

[maykonmenezes]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

Moving estimate default language job to sideqik

Related Tickets & Documents

#5305

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

[mstruve]

Thanks for another PR!!! Great start but there are a few additional pieces this needs to get it ready to go.

[mstruve]

Since we are not using a different service here I think we can remove this argument and explicitly call this service below in the worker.

Users::EstimateDefaultLanguage.call(user) if user

[maykonmenezes]

ok, I will change it

[mstruve]

Changing this job to a worker is great but you also need to update the call we make to use the new worker in the user model.

def estimate_default_language
  Users::EstimateDefaultLanguageWorker.perform_async(id)
end

Further, you will notice that this method is called in an after_create callback. At that point, the user is not committed to the database so the worker will not be able to find it. For this reason, we need to change the callback to after_create_commit

[maykonmenezes]

got it, I will do it

[rhymes]

Hi @maykonmenezes, thanks for the changes!

There are two failed specs: https://travis-ci.com/thepracticaldev/dev.to/builds/146065261

I think we should also lean on the safe side here and keep the old job in the code with this PR, less work for the SRE team that doesn't have to manually empty the queue.

Thank you again :)

[rhymes]

Hi @maykonmenezes, the two specs are still failing: https://travis-ci.com/thepracticaldev/dev.to/builds/146298922

ps. unless for integrating changes needed in a branch or to fix merge conflicts, in the contributing guide we encourage people not to force push after the initial reviews :)

[atsmith813]

Hey @maykonmenezes - thank you so much for your help on this! We're in the home stretch of completing the epic (#5305). Did you want to go ahead and implement the changes discussed above?

[atsmith813]

Hey @maykonmenezes! I didn't hear back so I went ahead and pushed up the changes since we're in the final stretch of the epic (#5305). Thanks again for getting the ball rolling on this - it was a huge help!

[mstruve]

LGTM 🚀

[rhymes]

Thank you @atsmith813 and thanks again @maykonmenezes for kickstarting all of this!

[mstruve]

I'm looking at this job and can't help but think about the LanguageDetector class in /labor that we use to detect article language. Feels like we should be detecting it the same way everywhere. @rhymes any thoughts on that? Should we make an #issue for it?

[rhymes]

I'm not sure I understood correctly your note but I think these classes are doing two different things (the naming could be improved):

• app/labor/language_detector.rb uses cld to infer language an article is written in

• app/services/users/estimate_default_language.rb uses a combination of settings and the user's twitter identity to guess which default language they would prefer reading in

There's a lot to be done for i10n in the app but I'm not sure these two classes could be merged