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

No more calling PHP code from LfMerge #173

Merged
merged 8 commits into from
Dec 8, 2021
Merged

Conversation

rmunn
Copy link
Collaborator

@rmunn rmunn commented Dec 7, 2021

This PR finally implements the updateCustomFieldViews logic in LfMerge, as part of the SetCustomFieldConfig method in MongoConnection (which is where the logic belongs anyway). This will allow us to move the lfmerge package into its own container in Language Forge, instead of having it so tightly coupled to the PHP container.


This change is Reviewable

This is the first step towards being able to get rid of the RunClass
code and not have our C# code call our PHP code.
Now that the MongoConnection knows how to update role views and user
views, this is no longer needed at all.
@rmunn rmunn force-pushed the feature/no-more-runclass-usage branch from 471c0a1 to f51090c Compare December 7, 2021 09:42
Copy link
Contributor

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

looks good so far!

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained (waiting on @rmunn)

@rmunn
Copy link
Collaborator Author

rmunn commented Dec 8, 2021

Just ran a test, and got Send/Receive to complete correctly, but the custom field ended up with a type of "basic" instead of "multitext". Should be pretty straightforward to figure out why, but this isn't quite ready to merge yet.

Did a bit of debugging and found that in addition to MultiString, we can
have a field type of String coming from FieldWorks for a custom field of
the most common type, which should convert to "multistring" in LF.
@rmunn rmunn marked this pull request as ready for review December 8, 2021 06:19
@rmunn
Copy link
Collaborator Author

rmunn commented Dec 8, 2021

Tested and working now, so now I can remove the bit from the Dockerfile that pulls in the web-languageforge image. That should make the lfmerge images much smaller and speed up the build time. Then it will be ready for review.

Copy link
Contributor

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained (waiting on @rmunn)

The logic from this method has moved into MongoConnection
@rmunn rmunn merged commit 6b59910 into master Dec 8, 2021
@rmunn rmunn deleted the feature/no-more-runclass-usage branch December 8, 2021 09:18
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.

2 participants