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

OptionList items use GUID as key (plus debug!) #1515

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

OptionList items use GUID as key (plus debug!) #1515

wants to merge 2 commits into from

Conversation

josephmyers
Copy link
Contributor

Fixes # (issue)

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Type of Change

Only keep lines below that describe this change, then delete the rest.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • migration required (a model change requiring a mongodb script or migration script)
  • UI change

Screenshots

Please provide screenshots / animations for any change that involves the UI. Please provide animations to demonstrate user interaction / behavior changes

Checklist

  • I have performed a self-review of my own code
  • I have reviewed the title/description of this PR which will be used as the squashed PR commit message
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

How to test

Please describe how to test and/or verify your changes. Provide instructions so we can reproduce. Please also provide relevant test data as necessary. These instructions will be used for QA testing below.

  • Test A
  • Test B

qa.languageforge.org testing

Testers: Check the box and put in a date/time to sign-off/attest the feature works as expected on qa.languageforge.org

  • Tester1 (YYYY-MM-DD HH:MM)
  • Tester2 (YYYY-MM-DD HH:MM)

@github-actions
Copy link

github-actions bot commented Oct 6, 2022 •
edited
Loading

Unit Test Results

368 tests   368 ✔️  13s ⏱️
    1 suites      0 💤
    1 files        0 ❌

Results for commit 330f1a7.

♻️ This comment has been updated with latest results.

@josephmyers josephmyers linked an issue Oct 6, 2022 that may be closed by this pull request
Status list in custom field not syncing correctly #1295
Open
megahirt
megahirt approved these changes Oct 28, 2022
View reviewed changes
Copy link
Collaborator

@megahirt megahirt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

From our in-person work on this issue, this is totally fine with me and is well documented. It's a small but important change to enable C# debugging in the LFMerge docker environment.

The only thing I might add is an addition comment on why elevated privileges are necessary. As best as I can remember (and it's already fading), privileged: true is needed in order to attach a debugger to a running process inside the docker container. This is an OS kernel-level feature not available in regular unprivileged containers. That's what I remember, anyway

@josephmyers
Updating LF OptionListItem's to use GUID's for keys, instead of the v…
b77f9cb 
…alue.

This was mostly already being done, but one place in the html (when the field is uneditable) and one in the php (when option list items are constructed without a given key).

The LFMerge side is still not working, but there aren't any obvious changes required yet in this repo.
@josephmyers josephmyers marked this pull request as ready for review November 15, 2022 19:43
longrunningprocess
longrunningprocess approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@longrunningprocess longrunningprocess left a comment

Choose a reason for hiding this comment

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

make sure to resolve that conflict and clean up the PR description a bit before you merge.

@megahirt
Copy link
Collaborator

make sure to resolve that conflict and clean up the PR description a bit before you merge.

Agreed - the PR is now doing more than just adding elevated privs for debugging. This is changing how keys are used in optionlists

@josephmyers josephmyers changed the title Giving lfmerge dev container elevated privileges to allow debugging OptionList items use GUID as key (plus debug!) Nov 17, 2022
@josephmyers
Copy link
Contributor Author

Don't worry, I'm not going to merge this til the LfMerge side is fixed, and it has a good bit more work to do

@laineyhm laineyhm force-pushed the develop branch 11 times, most recently from 3ebca62 to edc6ab4 Compare January 13, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@longrunningprocess longrunningprocess longrunningprocess approved these changes

@megahirt megahirt Awaiting requested review from megahirt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Status list in custom field not syncing correctly
3 participants
@josephmyers @megahirt @longrunningprocess