Sorry for the delays, I will try to get this tomorrow and post feedback on the merge request tomorrow.
This is a good opportunity to say that delays and time to discuss & coordinate are inevitable. Its a good reason to keep units of work small enough, and during "fulltime" work on Allura expect to switch gears to a different tasks on different branches while waiting for feedback. Thanks though and hope to post feedback tomorrow! On 5/4/18 3:47 PM, Deshani Geethika wrote: > Hi Dave, > > I've fixed the issue and added a merge request > <https://forge-allura.apache.org/p/allura/git/merge-requests/252/>. > > Please review it and let me know any improvements. > > Regards! > > On Fri, May 4, 2018 at 3:13 AM, Dave Brondsema <d...@brondsema.net> wrote: > >> I think this method, handling inbound emails to wiki pages, is probably >> not used >> very much :) I haven't tried it but it does seem like that would let >> people >> create new wiki pages just by emailing to the address. >> >> 1) We probably don't need to allow that. Just allow emails to make >> comments on >> *existing* wiki pages (which I believe handle_artifact_message does) >> >> 2) If you look at `git blame` for handle_message you can see that it is >> quite >> old. Some of the very early work on Allura doesn't make perfect sense any >> more. >> So in this case it probably would be fine to remove the try/except since >> as you >> say upsert() shouldn't raise any exception. And I don't think the >> underlying >> get() and upsert() calls within upsert() would raise any either. >> >> -Dave >> >> On 5/3/18 11:20 AM, Deshani Geethika wrote: >>> Hi Dave, >>> >>> Let's continue the discussion related to the above issue in this thread. >>> >>> I have few confusions on the forgewiki.wiki_main. >> ForgeWikiApp#handle_message >>> *[1]* method. Inside this method, Page.upsert method *[2]* is called, and >>> upsert method will update page with 'title' or insert new page with that >>> name. Does that means, if somebody sends an email to 'Foo_Baz@123' and >> if >>> that page is not available, a new page with the title 'Foo_Baz' will be >>> created? If so, we have some problems. >>> >>> 1. We can't validate the title >>> 2. 'upsert' method will never throw an exception (But in >> handle_message >>> method exception is handled) >>> >>> >>> *[1]* def handle_message(self, topic, message): >>> log.info('Message from %s (%s)', >>> topic, self.config.options.mount_point) >>> log.info('Headers are: %s', message['headers']) >>> try: >>> page = WM.Page.upsert(topic) >>> except: >>> log.exception('Error getting artifact %s', topic) >>> self.handle_artifact_message(page, message) >>> >>> *[2]* @classmethod >>> def upsert(cls, title, version=None): >>> """Update page with `title` or insert new page with that name""" >>> if version is None: >>> # Check for existing page object >>> obj = cls.query.get( >>> app_config_id=context.app.config._id, >>> title=title) >>> if obj is None: >>> obj = cls( >>> title=title, >>> app_config_id=context.app.config._id, >>> ) >>> Thread.new(discussion_id=obj.app_config.discussion_id, >>> ref_id=obj.index_id()) >>> return obj >>> else: >>> pg = cls.upsert(title) >>> HC = cls.__mongometa__.history_class >>> ss = HC.query.find( >>> {'artifact_id': pg._id, 'version': int(version)}).one() >>> return ss >>> >>> Regards! >>> >>> On Thu, May 3, 2018 at 3:21 AM, Dave Brondsema <d...@brondsema.net> >> wrote: >>> >>>> On 5/2/18 11:24 AM, Deshani Geethika wrote: >>>>> Hi Dave, >>>>> >>>>> Thanks for the information. I have few questions to be clarified. >>>>> >>>>> I think replacing spaces with underscores would be a good way to do it, >>>>> since on many wikis they are >>>>> interchangable. (They aren't for allura, but we could move towards >>>> that). >>>>> >>>>> Here do you mean that spaces and underscores in a title are considered >> as >>>>> same characters? >>>>> >>>>> For example : "The Title" is considered same as "The_Title". >>>> >>>> Yes that's what I was thinking. >>>> >>>>> >>>>> In this situation, users should not be allowed to create 2 wikis with >>>> above >>>>> titles, because we can't handle inbound emails for both scenarios. >>>> >>>> Good point, I hadn't thought of that. That does make this more >>>> complicated. >>>> Maybe it could be a followup step to prevent creating a page that >>>> conflicts with >>>> another one. And then even later on we could make URLs handle spaces >> and >>>> underscores interchangably. >>>> >>>> Anyone else have ideas about what would be best? >>>> >>>>> >>>>> Also, we have another problem here. >>>>> >>>>> Then this handle_message method could try finding a page for that >>>> message, >>>>> and if it >>>>> doesn't exist, it can convert any underscores back to spaces and then >> try >>>>> find >>>>> the wiki page under that name. >>>>> >>>>> Here do I need to check for all the possible number of combinations of >>>>> underscores and spaces to find out the exact wiki page? >>>> >>>> >>>> I was thinking if an email comes in for "Foo_Bar_Baz" first look for one >>>> titled >>>> "Foo_Bar_Baz" and then one titled "Foo Bar Baz". Trying every >> combination >>>> could >>>> get crazy. >>>> >>>>> >>>>> Regards! >>>>> >>>>> >>>>> >>>>> On Wed, May 2, 2018 at 7:17 AM, Dave Brondsema <d...@brondsema.net> >>>> wrote: >>>>> >>>>>> Cool. That email_address property should be the main one and changing >>>> it >>>>>> should >>>>>> reflect in the outgoing emails that are sent to subscribers after >>>>>> commenting or >>>>>> editing a wiki page. >>>>>> >>>>>> Allura also supports *inbound* emails on most artifacts including wiki >>>>>> pages. >>>>>> So someone could reply to the wiki email and it would be received by >>>>>> Allura and >>>>>> added as a comment on the wiki page. The >>>>>> forgewiki.wiki_main.ForgeWikiApp#handle_message method is what is >>>>>> responsible >>>>>> for that. So that code should be updated as well. I think replacing >>>>>> spaces >>>>>> with underscores would be a good way to do it, since on many wikis >> they >>>> are >>>>>> interchangable. (They aren't for allura, but we could move towards >>>>>> that). Then >>>>>> this handle_message method could try finding a page for that message, >>>> and >>>>>> if it >>>>>> doesn't exist, it can convert any underscores back to spaces and then >>>> try >>>>>> find >>>>>> the wiki page under that name. >>>>>> >>>>>> To test the inbound mails, I'm unfortunately not seeing any tests in >> the >>>>>> code. >>>>>> You could add some. The ForgeWiki/forgewiki/tests/test_app.py file >>>> has a >>>>>> TestBulkExport class and you could copy most of its setup, and then >> add >>>> a >>>>>> test_email test case that calls wiki.handle_message. >>>>>> >>>>>> The other way is to use telnet or other tools like that to send the >> mail >>>>>> into >>>>>> the "inmail" docker compose container, or `paster smtp_server` service >>>> if >>>>>> you >>>>>> aren't using docker. There's an example of doing that in the middle >> of >>>>>> this >>>>>> page: https://forge-allura.apache.org/p/allura/wiki/Notes/ >>>>>> >>>>>> Hope that helps! It sounds a little more complex of a ticket than I >>>>>> initially >>>>>> thought it would be. Let us know if you have any more questions or >> get >>>>>> stuck on >>>>>> anything. >>>>>> >>>>>> -Dave >>>>>> >>>>>> On 5/1/18 12:45 PM, Deshani Geethika wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> During last few days, I spent time on reading the Allura >> documentation >>>>>> and >>>>>>> on getting familiarized with Allura codebase. >>>>>>> >>>>>>> Then, I have started to work on the ticket - #1699 Fix incoming email >>>> for >>>>>>> wiki pages with space in the title >>>>>>> <https://forge-allura.apache.org/p/allura/tickets/1699/>. According >> to >>>>>> my >>>>>>> understanding, it is required to replace the spaces in the title with >>>>>> null >>>>>>> string (or with some character). Therefore, the getter method for >>>>>>> email_address which is in Page.class in >> ForgeWiki/forgewiki/model/wiki >>>>>> .py >>>>>>> should be changed as below. >>>>>>> >>>>>>> >>>>>>> @property >>>>>>> def email_address(self): >>>>>>> if context.app.config.options.get('AllowEmailPosting', True): >>>>>>> domain = self.email_domain >>>>>>> * self.title.replace(‘ ‘,’’) // Added line* >>>>>>> return '%s@%s%s' % (self.title.replace('/', '.'), >>>> domain, >>>>>>> config.common_suffix) >>>>>>> else: >>>>>>> return tg_config.get('forgemail.return_path') >>>>>>> >>>>>>> Could you tell me whether, do I need to modify any method other than >>>> the >>>>>>> above one? >>>>>>> >>>>>>> >>>>>>> Regards! >>>>>>> >>>>>>> On Wed, Apr 25, 2018 at 10:30 PM, Deshani Geethika < >>>>>>> deshanigeeth...@gmail.com> wrote: >>>>>>> >>>>>>>> Hi Dave, >>>>>>>> >>>>>>>> Thanks for the detailed explanation. I will start working on this >> and >>>>>> come >>>>>>>> back to you with my progress >>>>>>>> >>>>>>>> Regards! >>>>>>>> >>>>>>>> On Wed, Apr 25, 2018 at 9:55 PM, Dave Brondsema <d...@brondsema.net >>> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> On 4/24/18 11:14 AM, Deshani Geethika wrote: >>>>>>>>>> Hi Dave, >>>>>>>>>> >>>>>>>>>> As per GSoC official time-line, from 23rd April to 14th May period >>>> is >>>>>>>>>> considered as "Community Bonding Period". >>>>>>>>>> >>>>>>>>>> During this period I would like to finalize my design and separate >>>> my >>>>>>>>>> project into several tickets. Also, I would like to get more >>>>>>>>> familiarized >>>>>>>>>> with Allura code-base and Allura team. >>>>>>>>>> >>>>>>>>>> Could you guide me what would be the best way to start off with. >>>>>>>>>> >>>>>>>>>> Regards! >>>>>>>>>> >>>>>>>>> >>>>>>>>> Sounds like good goals for the community bonding period. >>>>>>>>> >>>>>>>>> I've added you as a developer on our self-hosted Allura project >>>>>>>>> https://forge-allura.apache.org/p/allura/ which means you can >> assign >>>>>>>>> tickets to >>>>>>>>> yourself, make new ones, update existing ones, etc. I'd recommend >>>>>> having >>>>>>>>> many >>>>>>>>> small incremental tickets (perhaps even smaller pieces of work than >>>> you >>>>>>>>> outlined >>>>>>>>> in the project proposal), so that its easy to manage them and >> review >>>>>>>>> them. And >>>>>>>>> of course you don't need to make them all right away :) >>>>>>>>> >>>>>>>>> To familiarize yourself with Allura, you can read more of the >>>>>>>>> documentation - >>>>>>>>> assuming you haven't read it all already ;) >>>>>>>>> https://forge-allura.apache.org/docs/ >>>>>>>>> >>>>>>>>> And working on Allura code itself is best. Find an existing ticket >>>> or >>>>>>>>> anything >>>>>>>>> you notice that could be made better, and make a fix for it. A >>>> really >>>>>>>>> easy one >>>>>>>>> that I could suggest is https://forge-allura.apache.or >>>>>>>>> g/p/allura/tickets/1699/ >>>>>>>>> >>>>>>>>> I've also noticed that our test suite has failed the past few >> times: >>>>>>>>> https://builds.apache.org/blue/organizations/jenkins/ >> Allura/activity >>>>>> It >>>>>>>>> probably is related to the "Make debug pages and post permalinks >> work >>>>>>>>> correctly >>>>>>>>> when behind a proxy" commit. You could take a look at fixing that >> if >>>>>> you >>>>>>>>> want. >>>>>>>>> Otherwise I will soon. >>>>>>>>> >>>>>>>>> Lastly, reviewing other people's work is a good way to get familiar >>>>>> with >>>>>>>>> the >>>>>>>>> code and best practices. I will have a fix for >>>>>>>>> https://forge-allura.apache.org/p/allura/tickets/6353/ coming >> soon, >>>> so >>>>>>>>> watch out >>>>>>>>> for that. You won't be able to merge my branch to master, but it >> can >>>>>> be >>>>>>>>> a good >>>>>>>>> way for you to learn from others. And any constructive feedback >>>> would >>>>>> be >>>>>>>>> welcome too, of course. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Dave Brondsema : d...@brondsema.net >>>>>>>>> http://www.brondsema.net : personal >>>>>>>>> http://www.splike.com : programming >>>>>>>>> <>< >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> *Deshani Geethika* >>>>>>>> Undergraduate at Department of Computer Science and Engineering >>>>>>>> Faculty of Engineering - University of Moratuwa Sri Lanka >>>>>>>> LinkedIn <https://www.linkedin.com/in/deshanigeethika/> | GitHub >>>>>>>> <https://github.com/deshanigtk> | Mobile - +94776383034 >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Brondsema : d...@brondsema.net >>>>>> http://www.brondsema.net : personal >>>>>> http://www.splike.com : programming >>>>>> <>< >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>>> -- >>>> Dave Brondsema : d...@brondsema.net >>>> http://www.brondsema.net : personal >>>> http://www.splike.com : programming >>>> <>< >>>> >>> >>> >>> >> >> >> >> -- >> Dave Brondsema : d...@brondsema.net >> http://www.brondsema.net : personal >> http://www.splike.com : programming >> <>< >> > > > -- Dave Brondsema : d...@brondsema.net http://www.brondsema.net : personal http://www.splike.com : programming <><
