Story 10: Content trigger

[Shriyanshagro]

Related to #25 and in continuation to #47

[coveralls]

Pull Request Test Coverage Report for Build 246

• 67 of 77 (87.01%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.2%) to 90.0%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/collective/ifttt/utils.py	59	60	98.33%
src/collective/ifttt/browser/content_trigger.py	8	17	47.06%

Totals
Change from base Build 238:	1.2%
Covered Lines:	216
Relevant Lines:	240

---

💛 - Coveralls

[datakurre]

@Shriyanshagro Now that you implemented rule creation code inside the form view class, how about refactoring those methods into a standalone functions? (The current implementation is not wrong, but refactoring would be just evolution for it.)

That might feel like significant amount of work, because you could no longer use self.something variables from the form view instance, but you should pass rule context, request and parameters as function arguments.

On the other hand, the resulting code would be easier to reuse, less magical and maybe also easier to test with smaller tests.

So, in practice, methods like self.add_rule, self.delete_rule etc should be moved to some "library module", eg. utils.py as collective.ifttt.utils.

[Shriyanshagro]

@datakurre Sure, I'll look into those code fragments. I'm not sure but seems like with this code refactoring, I should also bring in implementation for story 11 and 12. This would help me while refactoring.

[datakurre]

@Shriyanshagro Regarding some of the currently failing tests, it's good to know that the Plone site id in plone test setup is plone in lowercase.

I recall, that name is available for import as a string variable at

from plone.app.testing.interfaces import PLONE_SITE_ID

that detail is described here https://pypi.org/project/plone.app.testing/

[datakurre]

@Shriyanshagro Please,

• fix indentation in these scenarios and keywords; indent by 4 spaces
• why there is leading space in ' Successfully ...'

[datakurre]

• the current branch contains unnecessary empty file src/collective/ifttt/tests/test_content_trigger.py

[datakurre]

@Shriyanshagro You cannot assume that the portal root name is Plone.

You can get the portal object

portal = api.portal.get()
adding = portal.restrictedTraverse('+rule')

But possibly that is also available through adapter lookup (faster)

adding = getMultiAdapter((portal, self.request), name='+rule')

[datakurre]

Later you use multiadapter lookups, so it is also the right solution here.

This is probably also the reason why acceptance tests fail.

[datakurre]

@Shriyanshagro Tests seem to be passing, but there is minor QA issue preventing this to be green

/home/travis/build/collective/collective.ifttt/src/collective/ifttt/tests/test_utils.py:8:80: E501 line too long (86 > 79 characters)

[datakurre]

Interesting that you found __name__ for api.portal.get().__name__. More usual one found in existing code is api.portal.get().getId().

[datakurre]

Minor fix: do not use os.sep when joining web paths. It is \ on windows.

[datakurre]

os.sep is no no for web paths. Nowadays it is theoretical that someone runs Plone on Windows, but if one does, os.sep is ’\.

[datakurre]

Web is standard, so it should be safe to do '/'.join().

If you want to use a helper, you could use

from posixpath import join as urljoin
c = urljoin(a, b)

as in https://stackoverflow.com/a/15279799

It uses / as separator on every OS.