Consolidate tests

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.

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.

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

• The error I don't understand why the code is there to wait on Javascript
• The test failures numbers 1, 2, 5, 6, 7 work locally.
• 3, 4 are blocked on the questions in the IS.

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.

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.

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.😃

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

I raised:

• 🐛 Fix problems arising from "Consolidate tests" Active
• 📌 Improvements to DeclarationStorage Active
• 🐛 Declaration:status problem Active
• 📌 Missing tests Active (in the IS here, but I'd like to close this issue - it's busy enough already)

Nice, thanks Adam.

Thanks I will continue in 🐛 Fix problems arising from "Consolidate tests" Active

Also I raised 📌 Finish the declaration forms Active

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

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