V4.9.2

[RayOei]

Bumped to 4.9.2

• Layout improvements and fixes in transaction overview
• Added missing translation entries
• Improves [BUG]: Mollie API doesn't contain pricing information anymore #1178
• Includes hot fix ec17071

Note: merging to main release branch/trunk for future v4.x releases.

Update:

Cherry-pick #1169
Cherry-pick #1178
Cherry-pick # 1166
Fix #1179
Fix #1181
Fix #1080

[RayOei]

@twothreenine @lentschi FYI: see #1199 (comment) and onwards on the juggling of branches.
Goal is to end-up with a v4.x branch/trunk for future v4 releases.
PR #1203 is also targeted to merge onto v4.x.

[RayOei]

Added @robwa too, if you are able 😁

[lentschi]

FYI: see #1199 (comment) and onwards on the juggling of branches.

I'm a bit confused by the discussion in #1199 and the effect in this PR: Is this PR just a collection of cherry-picks from master?.

If so, I wonder if they are all really necessary. For example fixing typos like here doesn't really make sense to me in a legacy branch unless we want to support said legacy branch for a long time and thus do a lot of other cherry-picks until it's abandoned. Is that the case and the reason why you did it?

EDIT: Ah, sorry - just saw, that you're cherry-picking from 4.x, but still: Haven't these changes been reviewed elsewhere and if so, how can we find the reviews to check if the cherry-picks could cause trouble in this case?

[RayOei]

Cherry picking is done separately. I am reviewing those as we speak 😁 As soon as I re-push that branch, I encountered an issue, I will add the commits as well. Only a few but some do bother the FCAN users at this time. As long as v4 is not abandonded adding (minor) improvements is not a bad thing, in my book anyway.
The branches discussion was about being able to clearly separate v4.x and v5.x. As there were a few different ideas floating around in branches in the past, it got a bit messy. We settled now on using the v4.x one for future updates on v4.

Part of the fixing/improving here was also meant as a way to get a bit better acquinted with the code. It is not imperative but does provide a few minor tweaks. I did find an issue yesterday, I left a Mollie specific item in there by accident, which I am going to address soon. Again also part of me getting to understand the plugin idea a bit better.

As a side note: while cleaning up the release4 confusion I also killed the Mollie PR as part of the cleaning. I will reintroduce that soon, as that does provide a needed fix for payments and missing fees.

[RayOei]

@lentschi Any idea why this spec is failing? It doesn't locally.

[lentschi]

@lentschi Any idea why this spec is failing? It doesn't locally.

@RayOei Hm... Not right away.
I'd have to bring my local setup into this old branch's state first. But to be honest, I still don't quite see what this PR is about, so even if I did, I couldn't say why some test fails - so some general questions first:

1. As you seem to be actively working on it, shouldn't it's state be Draft? (It's not something that could be reviewed & merged right now, right?)
2. What is this branch v4.9.2? (Meaning how was it created and out of what?)
3. Shouldn't creating a branch v4.x simply be a matter of branching of from the v4.9.1 tag? (And if not, what would be the difference to what you're doing here?)

[RayOei]

1. no this should be the 'final' product
2. This is a follow-up version from 4.9.1 with some fixes and improvements. As Git shows it is a continuation of 4.9.1 which will merge back to 4.x.
3. This was earlier discussed with yksflip as there were originally two similar, already existing, 4.x branches and this was chosen as a way to clean up. Which would mean having a 4.x branch and a 5.x (which is master).

As for the test failing: "Order sends mail if min_order_quantity has been reached". It doesn't fail locally. It is part of the code I haven't looked into so I don't yet know what the flow should be and how this could fail. But it is somewhat confusing that it all passes when run locally. That should not be different, unless there is a reason it is and I missed that?

[lentschi]

As for the test failing: "Order sends mail if min_order_quantity has been reached". It doesn't fail locally.

Okay, I just set everything up and it does fail on my machine too - here are all the commands I executed to get there:

git clone git@github.com:foodcoops/foodsoft.git -s foodsoft_4.x
cd foodsoft_4.x/
git checkout v4.9.2
git show | head -1

# confirm you're on the same revision as this PR:
# -> commit 600c8382b2c85171e79f5fc708ae8bde508eb545

# then set everything up and run the tests:
docker compose -f docker-compose-dev.yml up -d mariadb
docker compose -f docker-compose-dev.yml run --rm foodsoft bundle install
docker compose -f docker-compose-dev.yml run --rm foodsoft bundle exec rake foodsoft:setup_development_docker
docker compose -f docker-compose-dev.yml run --rm mariadb mariadb --host=mariadb --password=secret --execute="CREATE DATABASE test"
docker compose -f docker-compose-dev.yml run --rm foodsoft bundle exec rake db:schema:load RAILS_ENV=test DATABASE_URL=mysql2://root:secret@mariadb/test?encoding=utf8mb4
docker compose -f docker-compose-dev.yml run --rm foodsoft bundle exec rake rspec-rerun:spec RAILS_ENV=test DATABASE_URL=mysql2://root:secret@mariadb/test?encoding=utf8mb4

... which ultimately leads to the same error as the GitHub pipeline:

  1) Order sends mail if min_order_quantity has been reached
     Failure/Error: expect(ActionMailer::Base.deliveries.count).to eq 1
     
       expected: 1
            got: 0
     
       (compared using ==)
     # ./spec/models/order_spec.rb:80:in `block (2 levels) in <main>'

So, the most likely cause for this to fail, is that you're not on a clean working directory of 600c838. Other than that it might be a race condition due the difference in processor speed or something, but that's rather unlikely.

[lentschi]

@RayOei

This is a follow-up version from 4.9.1 with some fixes and improvements

What exactly are those "some fixes and improvements" you're referring to here? (What was the process to pick them and who picked them?)

Let me mention one random example:
#1078 seems to be in master and in v4.9.2.
It's in master because I myself reviewed and tested it for 5.0.
But who did that for 4.x.?

IMO 4.x should only contain urgent hotfixes (and I don't think the sample I picked is one of them - I think that's rather a nice-to-have).
Everything else should only be released in 5.0 - that's where all our focus should be right now!

(And even if we did pick some commits into 4.x, then it's not a good idea to do it in a bulk PR. It should be separate cherry-pick PRs then.)

[RayOei]

They are picked on the basis that our foodcoop has a serious user problems with some issues besides the Mollie issue costing us money every week. All main issues are mentioned in the PR.
Secondly this was done as a way to get accustomed to FS without adding issues to the 5.x stream, or being in the way.
Thirdly: those were cherry-picked separetely but it doesn't make too much sense to have PR's for all those individual ones. Also because getting PR's handled takes a lot of time. Because of personal circumstances this took way more time to get finished than planned as the approach was discussed last year with yksflip who would do the review and provide some background support.
As some other Amsterdam coops are busy or planning to upgrade, mosty because of Mollie, the 4.9.x would provide a good starting point which they can use as long as 5.x is not ready yet. In time this version is no longer needed, obviously. But before that the Mollie plugin part also has to be added to 5.x. Which would be the next step.

[lentschi]

@RayOei If there are "serious user issues" then fixing them in 4.x is, of course, valid. 👍

However, I stand by what I wrote above:

[...] even if we did pick some commits into 4.x, then it's not a good idea to do it in a bulk PR. It should be separate cherry-pick PRs then