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

Rc/core make widow width available in session #1414

Merged
merged 6 commits into from
Aug 7, 2024
Merged

Rc/core make widow width available in session #1414

merged 6 commits into from
Aug 7, 2024

Conversation

Robert-Costello
Copy link
Contributor

@Robert-Costello Robert-Costello commented Jun 28, 2024 •
edited
Loading

Technical Summary

jira ticket
Related Formplayer PR
Related Web Apps PR

Safety Assurance

Safety story

Automated test coverage

QA Plan

QA Ticket

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

Duplicate PR

Automatically duplicate this PR as defined in contributing.md.

@Robert-Costello Robert-Costello changed the base branch from master to formplayer July 1, 2024 14:29
@Robert-Costello Robert-Costello mentioned this pull request Jul 1, 2024
Merged
3 tasks
@Robert-Costello Robert-Costello marked this pull request as ready for review July 1, 2024 18:47
shubham1g5
shubham1g5 requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@shubham1g5 shubham1g5 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 though we should be adding tests here or on FP to demonstrate the sample usage of this parameter.

src/main/java/org/commcare/modern/session/SessionWrapper.java Show resolved Hide resolved
@Robert-Costello
Copy link
Contributor Author

we should be adding tests here or on FP to demonstrate the sample usage of this parameter.

Yes, definitely. I wanted to make sure this approach was making sense before adding a test/tests.
What are we looking for in a test for a single datum like this, in high level terms? Do we want to add windowWidth to a mock query and verify that it gets added into the menu/form sessions as expected? Or a more narrow unit test where it's in a test session already and we're just retrieving and verifying the value?
One test that seems somewhat related is this testSessionDatumParser that looks at device_id.

@shubham1g5
Copy link
Contributor

shubham1g5 commented Jul 3, 2024 •
edited
Loading

Do we want to add windowWidth to a mock query and verify that it gets added into the menu/form sessions as expected?

Yes, I think this is what we want.

@Robert-Costello
Copy link
Contributor Author

duplicate this PR

@github-actions github-actions bot mentioned this pull request Jul 5, 2024
Merged
@Robert-Costello
Copy link
Contributor Author

@shubham1g5 I'd like to go ahead and merge this, since the test in the FP PR depends on the changes here. How does that sound to you?

@shubham1g5
Copy link
Contributor

shubham1g5 commented Aug 5, 2024 •
edited
Loading

It will break the master builds on FP if we merge this without merging FP changes as well, it's fine for tests on FP PR to fail until we can finalise things on that PR. When we are ready to merge we should merge this first and then before merging FP side, we should get the FP build passes with these changes.

shubham1g5
shubham1g5 reviewed Aug 6, 2024
View reviewed changes
src/main/java/org/commcare/session/SessionInstanceBuilder.java Outdated
TreeElement sessionMeta = new TreeElement("context", 0);

addData(sessionMeta, "deviceid", deviceId);
addData(sessionMeta, "appversion", appversion);
addData(sessionMeta, "username", username);
addData(sessionMeta, "userid", userId);
addData(sessionMeta, "drift", String.valueOf(drift));
addData(sessionMeta, "windowwidth", windowWidth);
Copy link
Contributor

Choose a reason for hiding this comment

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

windowwidth troubles me a bit, can we underscore it as window_width although inconsistent with other params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll change it to window_width

MartinRiese
MartinRiese reviewed Aug 6, 2024
View reviewed changes
src/main/java/org/commcare/modern/session/SessionWrapper.java Outdated
this.frame = session.getFrame();
this.setFrameStack(session.getFrameStack());
this.windowWidth = windowWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that covered by the call on line 57?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think you're right

shubham1g5
shubham1g5 approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@shubham1g5 shubham1g5 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, though we should address https://github.com/dimagi/commcare-core/pull/1414/files#r1705888450 before merging.

@Robert-Costello Robert-Costello merged commit 03d2fa8 into formplayer Aug 7, 2024
2 checks passed
@Robert-Costello Robert-Costello deleted the rc/core-make-widowWidth-available-in-session branch August 7, 2024 14:49
@ctsims
Copy link
Member

ctsims commented Oct 17, 2024

Sorry for the late jump in on this, but was the documentation updated in the core repos for how this new field works and what the value sets are? There's lot of discussion in the specs but I didn't see the outcome become clear anywhere.

This feature has a very, very high probability of creating confusing behaviors where two people receive different experiences in ways that are extremely opaque, and being overloaded for different purposes, so I want to make sure that we have all of the ways it works be extremely unambiguous.

@Robert-Costello
Copy link
Contributor Author

Robert-Costello commented Oct 17, 2024 •
edited
Loading

The docs need to be updated here https://github.com/dimagi/commcare-core/wiki/commcaresession. I'll update it today.
Differing behaviors will come from app building that specifically utilizes this property in display conditions and I'll make that unambiguous in the docs.

@shubham1g5 @MartinRiese Is there any other place you're aware of that documentation should be updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinRiese MartinRiese MartinRiese approved these changes

@shubham1g5 shubham1g5 shubham1g5 approved these changes

@snopoke snopoke Awaiting requested review from snopoke

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

Successfully merging this pull request may close these issues.
4 participants
@Robert-Costello @shubham1g5 @ctsims @MartinRiese