Once #3504039: Consolidate contrib module setup is done, we should know if any of the current tests are failing. If they are, fix them.

Additionally, as part of #3504036: Document features please determine if any features are missing substantial test coverage and document what's missing here.

Non-obvious Fixes

Please check my proposals/assumptions.

  1. In GiftAidRelatedContextsSubscriber I removed the test against a draft order because for sure the order is draft during the checkout process when looking for an existing declaration.
  2. Charity has accessor methods for some fields that no longer exist so I removed them. The alternative of course would be to put the fields back.
  3. Currently I have disabled the code in Declaration::postSave() for "If this declaration has had its end date edited, force the same end date on other ongoing declarations for the same context." Firstly the consequences were baffling me for many hours and I probably won't be alone😃. Secondly it seems unnecessary as the separate declarations will do the right thing anyway. Thirdly I am unsure about the legal compliance: it seems better to report to HMRC the declaration exactly as they were made. If someone chooses to extend a declaration we should do what they ask; if they choose to make a new declaration we should also do what they ask. Fourthly (obviously fixable) it seems bugged: a declaration for 2023 plus one for 2025 would wrongly also cover 2024. Finally consider the case that a declaration is added by mistake, then shortly after removed - it could have corrupted many other declarations. I disabled the corresponding test.
  4. I simplified the code in DeclarationStorage::getActiveByAnyContext() so it just returns the most recently declared, and removed the corresponding tests. It seems that it shouldn't really matter which one we return if multiple are relevant, and I couldn't conceive of any rule relating to start/end dates that actually works reliably.

Questions/problems

Please let me know what you prefer, or we can discuss.

  1. The routes gift_aid.declaration_make and gift_aid.declaration_extend are apparently missing the corresponding form. I'm not sure if they are still needed - potentially we could reuse the add and edit forms??
  2. DeclarationFormTest has code that expects special button text changes and thank-you messages, however I can't see corresponding code to generate them. Either I can change the tests or add code to match.
  3. I'm not sure that the calculated field Declaration:status is going to work. The value depends on the date, so it would break any caching. Also I'm not sure we need it: the function getStatus() is fine for most purposes. I believe we can rewrite the various functions in DeclarationStorage to work without it - and at the same time be faster, and take an extra parameter of a timestamp. This seems important as we can't always assume the current date is the correct perspective: for example suppose someone adds an order in the past or edits the date or an order. (This one isn't breaking any tests - it's just something I spotted.)

Issue fork gift_aid-3504041

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathanshaw created an issue. See original summary.

adamps’s picture

adamps commented
Version: » 1.x-dev

  • adamps committed a671c5df on 1.x 
    Issue #3504041 by adamps: Consolidate tests
adamps’s picture

adamps commented

I pushed a first commit without a MR, because the MR system didn't seem to be available.

However it seems to be available now, likely because the version field is now set. So if my first commit wasn't enough then I should be able to create a MR for the next step.

adamps opened merge request !1

  • adamps committed 71b52e68 on 1.x 
    Issue #3504041 by adamps: Consolidate tests

adamps opened merge request !2

adamps’s picture

adamps commented
Issue summary: View changes
adamps’s picture

adamps commented
Assigned: adamps » jonathanshaw
Issue summary: View changes

You set me some interesting challenges to guess how you intended the code to be😃. I'm getting close to the limit now of what I can do until you can clarify/resolve some uncertainties please.

adamps’s picture

adamps commented

In https://git.drupalcode.org/project/gift_aid/-/jobs/4280333

  • DELETED IT The error I don't understand why the code is there to wait on Javascript
  • FIXED The test failures numbers 1, 2, 5, 6, 7 work locally.
  • 3, 4 are blocked on the questions in the IS.
  • I don't see the 10 warnings locally.
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

Nice work! And challenging :)

I'm fine to leave commited the changes you've made, but some need further discussion in new more specific issues, ideally with the removed code in the IS for consideration, or a link to the commit in the MR here.

In GiftAidRelatedContextsSubscriber I removed the test against a draft order because for sure the order is draft during the checkout process when looking for an existing declaration.

New issue. Your point is valid, but I'm not sure a past declaration on a draft order should be considered to have been truly declared.

disabled the code in Declaration::postSave() for "If this declaration has had its end date edited, force the same end date on other ongoing declarations for the same context."

New issue please. Your points are good, but I had a good reason we should consider how to address.

I simplified the code in DeclarationStorage::getActiveByAnyContext() so it just returns the most recently declared, and removed the corresponding tests. It seems that it shouldn't really matter which one we return if multiple are relevant, and I couldn't conceive of any rule relating to start/end dates that actually works reliably.

New issue. That code looks ok to me.

The routes gift_aid.declaration_make and gift_aid.declaration_extend are apparently missing the corresponding form. I'm not sure if they are still needed - potentially we could reuse the add and edit forms??

Let's remove extend, I can't see an important case for extending a declaration rather than just making a new one.

For make, please open a new issue.

DeclarationFormTest has code that expects special button text changes and thank-you messages, however I can't see corresponding code to generate them. Either I can change the tests or add code to match.

I think this is what I was working on when I stalled 2 years ago. I suggest opening a new issue, and give some consideration to what the form should be like. Hopefully it's just changing it to match the tests, but might be more complex. Happy for you to also do simple changes here to get the tests passing if you prefer.

I'm not sure that the calculated field Declaration:status is going to work. The value depends on the date, so it would break any caching. Also I'm not sure we need it: the function getStatus() is fine for most purposes.

New issue. We could appropriate cache expiry to the entity. I guess the purpose of it was for display (a computed field being more elegant than hook_entity_extra_field), but I'm open to other approaches to displaying status.

I believe we can rewrite the various functions in DeclarationStorage to work without it - and at the same time be faster, and take an extra parameter of a timestamp. This seems important as we can't always assume the current date is the correct perspective: for example suppose someone adds an order in the past or edits the date or an order. (This one isn't breaking any tests - it's just something I spotted.)

New issue. Sounds like a good idea.

Undefined array key "wrapper_element"

That's an annotation on a commerce checkout pane plugin.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

Charity has accessor methods for some fields that no longer exist so I removed them. The alternative of course would be to put the fields back.

New issue please. I'm not sure what's right here, that might get clearer. I suspect we need to display the charity number and address to users at some point.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Assigned: jonathanshaw » adamps
adamps’s picture

adamps commented

That's an annotation on a commerce checkout pane plugin.

Yes I considered that, but there is a default value in the pane manager so it should be fine. Eventually I discovered that PaneVisibilityTest::getPane() creates the pane without calling the manager, so it ignores the annotation.😃

adamps’s picture

adamps commented
Assigned: adamps » jonathanshaw
Status: Active » Needs review

Hurrah, tests are green. I'm ready to commit, so please do any more review if you wish.

I raised:

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs review » Reviewed & tested by the community

Nice, thanks Adam.

adamps’s picture

adamps commented
Status: Reviewed & tested by the community » Fixed

  • adamps committed bf5d6085 on 1.x 
    Issue #3504041 by adamps, jonathanshaw: Consolidate tests
adamps’s picture

adamps commented

Thanks I will continue in #3505391: Fix problems arising from "Consolidate tests"

adamps’s picture

adamps commented
Assigned: jonathanshaw » Unassigned
adamps’s picture

adamps commented

Also I raised #3505467: Finish the declaration forms

adamps’s picture

adamps commented
Assigned: Unassigned » adamps
Status: Fixed » Needs work

Amusingly and rather confusingly, the tests fail in the morning and pass in the afternoon😃. Fix coming up soon.

adamps opened merge request !3

  • adamps committed 17b70b8c on 1.x 
    Issue #3504041 by adamps, jonathanshaw: Consolidate tests (fix...
adamps’s picture

adamps commented
Status: Needs work » Fixed
adamps’s picture

adamps commented
Assigned: adamps » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.