Skip to content

fix bug related to missing contact_method information in some cases #27

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
Merged
markgoho merged 4 commits into from
Dec 16, 2021
Merged

fix bug related to missing contact_method information in some cases #27

markgoho merged 4 commits into from
Dec 16, 2021

Conversation

@Battula
Copy link
Contributor

@Battula Battula commented Dec 16, 2021 •
edited by markgoho
Loading

@Battula
Updates related to FRE notice
3d2ea17 
 - Updated aca_entities gem version
 - Updated preview payload to include health and dental enrollments
 - Fixed bug related to missing contact_method information in some cases
 - Updated verabiage for Cub Care in application settings
markgoho
markgoho suggested changes Dec 16, 2021
View reviewed changes
Copy link
Contributor

@markgoho markgoho left a comment

Choose a reason for hiding this comment

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

please update PR title to be less generic

@Battula
Copy link
Contributor Author

Battula commented Dec 16, 2021

@markgoho like ?

@Battula Battula requested a review from markgoho December 16, 2021 21:56
@markgoho
Copy link
Contributor

markgoho commented Dec 16, 2021

@Battula this is up to you as the developer to describe your changes in a way that conveys meaning and summarizes what's changing about the code

what you've written amounts to "made changes" -- yeah, okay, that's what a PR does, but what changes are you making?

If you're finding it difficult to summarize the changes, it's possible you've put too much in the PR and it needs to be broken out in to separate changes, each able to be independently reviewed

@Battula
Copy link
Contributor Author

Battula commented Dec 16, 2021 •
edited
Loading

@markgoho I have already summarized the list of changes in the commit message.

If you're finding it difficult to see the list of changes that I made, please click here.

The changes aren't too big to break them down in to separate PRs. Thanks

@markgoho
Copy link
Contributor

markgoho commented Dec 16, 2021

@Battula I'm not finding it difficult to understand the changes, I'm finding it difficult to see how "made changes" properly summarizes a number of distinct changes

options:

  1. find a better title that better summarizes the changes
  2. break the PR up into multiple PRs

@Battula
Copy link
Contributor Author

Battula commented Dec 16, 2021

@markgoho I can't summarize all the changes in the PR title, so I came up with a generic title and summarized the changes in the comment section.

@markgoho
Copy link
Contributor

markgoho commented Dec 16, 2021

@Battula please submit PRs for each of these:

  • Updated aca_entities gem version
  • Updated preview payload to include health and dental enrollments
  • Fixed bug related to missing contact_method information in some cases
  • Updated verabiage for Cub Care in application settings

@markgoho markgoho closed this Dec 16, 2021
@Battula Battula reopened this Dec 16, 2021
@Battula
Copy link
Contributor Author

Battula commented Dec 16, 2021 •
edited
Loading

@markgoho May I please know what's wrong with this PR ?

@markgoho
Copy link
Contributor

markgoho commented Dec 16, 2021

@Battula this PR clearly combines 4 different changes to the codebase that each need their own PR

here are the PRs I expect to see:

  • Updated aca_entities gem version
  • Updated preview payload to include health and dental enrollments
  • Fixed bug related to missing contact_method information in some cases
  • Updated verabiage for Cub Care in application settings

@markgoho markgoho closed this Dec 16, 2021
@Battula
Copy link
Contributor Author

Battula commented Dec 16, 2021

All the changes are related to FRE notice, and I don't see why they can't be on a single pull request. @markgoho
They aren't too big to split them into separate PRs.

@Battula Battula reopened this Dec 16, 2021
@Battula Battula enabled auto-merge (squash) December 16, 2021 22:23
@markgoho markgoho closed this Dec 16, 2021
auto-merge was automatically disabled December 16, 2021 22:26

Pull request was closed

@Battula Battula reopened this Dec 16, 2021
@Battula
Copy link
Contributor Author

Battula commented Dec 16, 2021

@markgoho Please don't close this PR, we need this to be merged to trunk. Thank you!

@markgoho markgoho changed the title updates related to FRE notice fix bug related to missing contact_method information in some cases Dec 16, 2021
@markgoho markgoho enabled auto-merge (squash) December 16, 2021 23:43
@markgoho markgoho merged commit 6393cec into trunk Dec 16, 2021
@markgoho markgoho deleted the 180463890_FRE_notice_updates branch December 16, 2021 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harshaellanki harshaellanki harshaellanki approved these changes

+1 more reviewer

@markgoho markgoho markgoho approved these changes

Reviewers whose approvals may not affect merge requirements

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

@Battula @markgoho @harshaellanki