Skip to content

Conversation

@hoffie
Copy link

@hoffie hoffie commented Sep 28, 2015

The internal attribute Item._folder should always be initialized,
at least with None.
If possible, _folder should be pre-populated with parent data from
the outside as this should be better performing than doing a repeated lookup.

Fixes #31.

The internal attribute Item._folder should always be initialized,
at least with None.
If possible, _folder should be pre-populated with parent data from
the outside as this should be better performing than doing a repeated lookup.

Fixes zarafagroupware#31.
@jelly
Copy link
Collaborator

jelly commented Sep 28, 2015

I agree that it is a bug, but I think we should fix it either in the init() or add a check in the Item.folder() code to check if the store exists. So that Item creation with a mapiobj is a bit less buggy.

The last solution also reveals the following bug

AttributeError: 'Item' object has no attribute 'store'

So I guess we then need to set the store explicitly in the init().

@hoffie
Copy link
Author

hoffie commented Sep 29, 2015

Thanks for the fast reply. I am a bit unsure whether you are actually suggesting the same what I proposed or something different. :)

To clarify, I would suggest to initialize _folder in __init__() (or as part of the class definition) as Item.folder() is just one possible place for errors due to this behaviour. Future code might introduce similar bugs.

In addition, I only propose to pre-initialize new Item objects with a parent if possible as this should help with performance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'self' is a Store, so not really the direct parent of the item.. but note that 'Item.folder' looks up the correct parent folder using PR_PARENT_ENTRYID.

@srepmub
Copy link
Collaborator

srepmub commented Sep 29, 2015

alright, jelle, so I guess we want to merge the Item(parent=self) changes when Item() is called from a Folder method. and probably it should be Item(folder=self) by now.

@hoffie
Copy link
Author

hoffie commented Sep 29, 2015

@srepmub, yes, one Item(folder=self) instance seems to be wrong as you noted.
From your comments I assume that this repository is out of date compared to ZCP 7.2? So your primary repository is located somewhere internally? My assumption was that the GitHub version was newer and 7.2 just contained a stable snapshot it of.

Is there anything else I should do with the pull request?

@jelly
Copy link
Collaborator

jelly commented Oct 13, 2015

Looks good, we will merge this when the repo is up to date

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants