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

Convert upload client to kotlin #5568

Merged

Conversation

psh
Copy link
Collaborator

@psh psh commented Feb 20, 2024

Given the centrality and size of the UploadClient this PR was split out the work of converting clients to Kotlin to make sure all is good and verifiable.

Getting good tests around the UploadClient meant some refactoring

  • to pull out static references to other classes
  • to go down the road of making the Contribution class a little more OO by encapsulating logic that was touching its internal state
  • constructing FileUtilsWrapper with a context (provided by Dagger in the real production code) to remove the context from the helper method interface

Once the test was in place around the Java version, it provided safety to convert the Java implementation to Kotlin and verify that none of its behavior had changed.

Note: this relates to the discussion in #4665

Comment on lines +54 to +55
return new UploadClient(uploadInterface, csrfTokenClient, pageContentsCreator,
fileUtilsWrapper, gson, System::currentTimeMillis);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The functional interface TimeProvider is designed to let us pass a lambda that returns a Long value e.g.

{ System.currentTimeMillis() }

Which then simplifies to the method reference you see here. All this facilitates massing a mock<TimeProvider>() in unit tests giving us control of the time we use in the upload client 😄

https://kt.academy/article/ek-function-types

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Tried 1 single upload and 2 multi-uploads, seems to work normally.
Thanks a lot! :-)

@nicolas-raoul nicolas-raoul merged commit f0a1d03 into commons-app:main Feb 21, 2024
1 check 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.

None yet

2 participants