Fixes #66

[lentschi]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 9730921185

Details

• 20 of 36 (55.56%) changed or added relevant lines in 1 file are covered.
• 1674 unchanged lines in 102 files lost coverage.
• Overall coverage decreased (-24.6%) to 43.328%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/helpers/articles_helper.rb	20	36	55.56%

Files with Coverage Reduction	New Missed Lines	%
app/serializers/group_order_article_serializer.rb	1	80.0%
plugins/messages/app/mail_receivers/messages_mail_receiver.rb	1	20.93%
app/controllers/api/v1/configs_controller.rb	1	75.0%
app/models/bank_account.rb	1	63.16%
app/controllers/application_controller.rb	1	87.5%
app/models/invoice.rb	1	74.19%
app/models/membership.rb	1	85.71%
app/serializers/order_article_serializer.rb	1	83.33%
app/controllers/api/v1/user/users_controller.rb	1	75.0%
app/mail_receivers/bounce_mail_receiver.rb	1	44.44%

Totals
Change from base Build 9714447341:	-24.6%
Covered Lines:	2955
Relevant Lines:	6820

---

💛 - Coveralls

[yksflip]

hey, code looks good to me! I've added two commits, please see them as suggestions, if you don't like it, feel free to drop them again ;)
95aaa00: I tried to reduce the code duplication in articles_helper_spec.rb
b078cfa: I made helper functions that are only called from within articles_helper.rb private

It took me some time to get what format_unit_factor does, maybe a comment with the general idea described in #66 could help here. Also the article_version.send(unit_property) logic was at first a bit confusing, but I see why you implemented it this way and it's actually quite nice.

[coveralls]

Pull Request Test Coverage Report for Build 9760316565

Details

• 40 of 40 (100.0%) changed or added relevant lines in 1 file are covered.
• 37 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.2%) to 67.669%

---

Files with Coverage Reduction	New Missed Lines	%
app/helpers/articles_helper.rb	1	84.72%
app/lib/order_csv.rb	1	88.89%
app/documents/order_fax.rb	10	77.14%
app/helpers/orders_helper.rb	11	85.32%
app/lib/order_txt.rb	14	22.22%

Totals
Change from base Build 9714447341:	-0.2%
Covered Lines:	4615
Relevant Lines:	6820

---

💛 - Coveralls

[lentschi]

@yksflip Thanks for you review!

Also thanks for your additions. You're right about both issues you addressed, of course.

95aaa00: I tried to reduce the code duplication in articles_helper_spec.rb

Yeah, sorry I was aware of the ugliness of the duplication. The reason I didn't change it, was that I thought, it'd be best to move the test_with_article_data / setup_article_version logic to the article (version) factory somehow. This would then help reduce duplication in other specs to - for example see

foodsoft_hackathon/spec/models/article_version_spec.rb

Lines 92 to 98 in c73596b

	it 'allows converting piece units if ratios are defined accordingly' do
	article_version.supplier_order_unit = 'XCR'
	article_version.article_unit_ratios = [
	ArticleUnitRatio.new({ sort: 0, unit: 'XBO', quantity: 20 }),
	ArticleUnitRatio.new({ sort: 1, unit: 'LTR', quantity: 10 })
	]
	article_version.save

But until I find time to do that, I'll keep your changes. 👍

It took me some time to get what format_unit_factor does, maybe a comment with the general idea described in #66 could help here.

Hm... I'm usually a big fan of self-explanatory code as I think code comments are seldom maintained properly. They can thus become misleading at a later point so I try to avoid them for everything but external API or DB fields etc. Maybe we can just think of a better name for the method making its purpose more obvious?
(The domain specific name "unit factor" I invented here is probably not perfect. Can you think about something better for the "3" and "12 x glass" parts in a unit format string such as "3 x 12 x glass"?)
But I do agree that this is quite a complex matter, so it might be okay to make an exception here and add some RDoc 🤔
It could also help to extract the whole unit formatting to a separate helper/lib, but I didn't want to over-engineer.

b078cfa: I made helper functions that are only called from within articles_helper.rb private
[...]
the article_version.send(unit_property) logic was at first a bit confusing, but I see why you implemented it this way and it's actually quite nice.

Well I'm not sure, if it's that nice and was actually thinking about changing it too. The only reason I introduced the format_XXX_unit_with_ratios methods, was because I want the future developers to have a clear interface: If they would call format_unit_with_ratios directly from the view, with something like format_unit_with_ratios(:supplier_TYPO_unit) the error would be unpredictable (something about send inside the helper not finding supplier_TYPO_unit) whereas format_TYPO_unit_with_ratios() would lead to a clear undefined method inside the view code.

I'll keep that change too and think about it later (Could still be changed easily throughout the application by running copy/replace). 👍

[twothreenine]

Looks good to me apart from the minor thing with the multiplier.

[lentschi]

Looks good to me apart from the minor thing with the multiplier.

Sorry, which "minor thing with the multiplier" are you referring to?

[twothreenine]

See my review above

[lentschi]

See my review above

Could you please post a citation & link - I'm not seeing it. (Maybe it's hidden somehow?)

[coveralls]

Pull Request Test Coverage Report for Build 9910240083

Details

• 40 of 40 (100.0%) changed or added relevant lines in 1 file are covered.
• 37 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.2%) to 67.669%

---

Files with Coverage Reduction	New Missed Lines	%
app/helpers/articles_helper.rb	1	84.72%
app/lib/order_csv.rb	1	88.89%
app/documents/order_fax.rb	10	77.14%
app/helpers/orders_helper.rb	11	85.32%
app/lib/order_txt.rb	14	22.22%

Totals
Change from base Build 9714447341:	-0.2%
Covered Lines:	4615
Relevant Lines:	6820

---

💛 - Coveralls