Mobile: Remove duplicate appointments reason_code logic

[Tonksthebear]

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

Reason code text is not being parsed at the appointments service level. This PR remove mobile's processing as it is now taken care of upstream of our adapter

Related issue(s)

department-of-veterans-affairs/va-mobile-app#9414

Testing done

Edited tests to no longer test our logic as the service that parses reason code performs the testing now

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)

Acceptance criteria

• I fixed|updated|added unit tests and integration tests for each feature (if applicable).
• No error nor warning in the console.
• Events are being sent to the appropriate logging solution
• Documentation has been updated (link to documentation)
• No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
• Feature/bug has a monitor built into Datadog (if applicable)
• If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
• I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

[Tonksthebear]

This feels like a risky PR. Our request specs don't actually thoroughly test the different types of appointments with the expected reason code parsing, so we're relying on modules/vaos/spec/services/v2/appointments_reason_code_service_spec.rb to cover those use cases.

Our appointments request spec needs to be cleaned up, but, outside of that, I'm not sure I added enough here to feel confident. Would love to know if I'm overthinking the risk of this pr

[kpethtel]

I think you should get rid of that unnecessary begin block, but otherwise this looks good.

I agree with you that this is a little risky because of the weak specs. As I mentioned on requested periods, it isn't clear to me that the new logic matches the old logic. You clearly put more time into investigating it than I did, so I trust you on that.

How confident do you feel that this matches the old behavior? I think if you feel confident, this is good. It doesn't seem likely to cause outright errors but may cause inconsistent data. Obviously you should test it on staging, but even that won't catch data issues unless you find the right data to test.

[kpethtel]

This is fine, but this method no longer performs any logic and is fundamentally just an attr_reader and could just be replaced with appointment[:requested_periods] where the function is used, which is consistent with how other simple appointment attributes are used in this file.

Looking at the appointments service logic, it looks to me like removing this will change functionality. The service uses the requested periods to do a number of things, but I don't see where it overwrites the requested periods with the preferred dates. I don't know if that's a problem or not. (Take what I'm saying here with a grain of salt because I'm sure you looked at this more closely than I did.)

[Tonksthebear]

I agree removing is changing functionality. We seem to look for requested periods in the reason_code whereas the service expects it to exist as an attribute. I'm not sure if that's a big deal or if they did it that way because requested periods no longer exist in the reason code

[Tonksthebear]

Confirmed that the change this makes is just swapping the priority of reason code data vs attribute data. Should be good to go

[github-actions]

Calls 'appointment[:requested_periods]' 2 times - DuplicateMethodCall

[github-actions]

Performs a nil-check - NilCheck

[Tonksthebear]

I'm going to look again with fresh eyes tomorrow to make sure everything looks good. I'm definitely still quite hesitant on this, especially because of how lacking the tests are. It at least won't cause errors like you said so I'm confident it won't break functionality, may just break some data

[kpethtel]

🚀

[Tonksthebear]

I'm going to wait until Monday to merge this to play it safe

[Tonksthebear]

NVM waiting for someone to review this so that they can merge it since we're no longer able to merge our own PRs