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

DYN-5733 fix language CLI argument #15739

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Conversation

chubakueno
Copy link
Contributor

@chubakueno chubakueno commented Dec 31, 2024

Purpose

Fix language CLI -l locale argument to override user preferences, which is currently not functional even though its documented.
New behavior with spanish preferred language:
2024-12-31 07-47-12
New behavior with no preferred language (default english):
2024-12-31 08-51-24

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Fix CLI -l locale argument to override user language preferences.

Reviewers

@QilongTang
@RobertGlobant20

FYIs

@avidit

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-5733

@chubakueno chubakueno changed the title DYN-5733: fix language CLI argument DYN-5733 fix language CLI argument Dec 31, 2024
Copy link

github-actions bot commented Dec 31, 2024

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

/// <param name="info">Host analytics info specifying Dynamo launching host related information.</param>
/// <returns></returns>
public static DynamoModel MakeCLIModel(string asmPath, string userDataFolder, string commonDataFolder, HostAnalyticsInfo info = new HostAnalyticsInfo())
public static DynamoModel PrepareModel(
Copy link
Member

@mjkkirschner mjkkirschner Jan 2, 2025

Choose a reason for hiding this comment

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

you cannot break these public api signatures of a public class.

Copy link
Contributor

@RobertGlobant20 RobertGlobant20 Jan 2, 2025

Choose a reason for hiding this comment

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

@chubakueno I guess a better option should be creating an overloaded for public static DynamoModel MakeModel which receives string CLIlocale as parameter so in this way you won't need to modify the existing public method (I think default argument is also NOT an option).

Copy link
Contributor Author

@chubakueno chubakueno Jan 3, 2025

Choose a reason for hiding this comment

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

Done, though there are now two(previously there already was one) unused overloads of the methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chubakueno I can see that now we have three MakeModel() methods and only two of them are calling PrepareModel(), is possible to also use PrepareModel() in the missing one?
image

Copy link
Contributor Author

@chubakueno chubakueno Jan 3, 2025

Choose a reason for hiding this comment

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

Done. Behavior before was not identical to the other overloads so PrepareModel wasn't an equivalent refactor (no calls to AssignHostPathAndIPathResolver, SetUICulture, OnDetectLanguage, OnRequestUpdateLoadBarStatus), but that MakeModel is only used by integration tests and the lack of those calls is likelier than not to be an unintentional omission so i think it's a good idea that all exhibit the same behavior.

Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a comment

Choose a reason for hiding this comment

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

LGTM with one comment

/// <param name="info">Host analytics info specifying Dynamo launching host related information.</param>
/// <returns></returns>
public static DynamoModel MakeCLIModel(string asmPath, string userDataFolder, string commonDataFolder, HostAnalyticsInfo info = new HostAnalyticsInfo())
public static DynamoModel PrepareModel(
Copy link
Contributor

Choose a reason for hiding this comment

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

@chubakueno I can see that now we have three MakeModel() methods and only two of them are calling PrepareModel(), is possible to also use PrepareModel() in the missing one?
image

@QilongTang QilongTang added this to the 3.5 milestone Jan 3, 2025
@QilongTang
Copy link
Contributor

Waiting for PR checks

@chubakueno
Copy link
Contributor Author

chubakueno commented Jan 3, 2025

Waiting for PR checks

Finished now, looks like UI Smoke Tests failed even though the only change since successful 6494d89 was changing the method to private. Self Serve executed succesfully.

@QilongTang
Copy link
Contributor

hi @avidit The test AgreementsForDataCollection failed for this PR, how to tell if that is real?

@avidit
Copy link
Contributor

avidit commented Jan 6, 2025

hi @avidit The test AgreementsForDataCollection failed for this PR, how to tell if that is real?

It was a timeout issue. I have verified the test case manually.

@chubakueno
Copy link
Contributor Author

chubakueno commented Jan 6, 2025

Hi @QilongTang , seems the automated UI test didn't click the Launch Dynamo button properly because of a flaky UI interaction: https://sharereports.smartbear.com/FFBC06CDBA7D49648DD30D76D06B49A801/result . Rerun the test and it's now fully green.
image

@QilongTang
Copy link
Contributor

Thank you @avidit @chubakueno for validation.

@QilongTang QilongTang merged commit 10df94f into DynamoDS:master Jan 7, 2025
24 checks passed
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.

5 participants