Git Merge Request #143: Attachments listing and lightbox added to discussion tool (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Rohan Verma wants to merge 5 commits from /u/rhnvrm/allura/ to master, 2016-08-01

Preview:
Imgur

Please review and suggest changes.

Commit Date  
[560107] (rhnvrm/design/view-attachment) by Rohan Verma Rohan Verma

Update test_forum:testposts test with updated html

2016-07-30 13:58:49 Tree
[f8fb80] by Rohan Verma Rohan Verma

Updated code as per suggestions in review

2016-07-26 08:38:46 Tree
[da54c9] by Rohan Verma Rohan Verma

Updated Tests to match changes

2016-07-13 12:51:22 Tree
[9c88c1] by Rohan Verma Rohan Verma

Readd old attachment css

2016-07-12 22:51:22 Tree
[4c71ed] by Rohan Verma Rohan Verma

Initial design changes to list attachments. Added lightbox_me to view images

2016-07-12 01:43:22 Tree

Discussion

  • Dave Brondsema

    Dave Brondsema - 2016-07-22

    Looks a lot better :D

    It would be great to have this applied to other attachment areas, like wiki pages and tickets. That's fine to be done later separately though.

    • <a href="http://localhost:8080/p/test/tickets/1/#dd1a"... is hard-coded in there
    • deleteAttachment references a myform form that isn't defined anywhere, probably can remove that function?
    • there's also a little onLoad function that is empty and can be cleaned up
    • for long filenames, they get chopped off. Something like word-wrap: break-word; would help so you can always see the full filename.
     
    • Rohan Verma

      Rohan Verma - 2016-07-26

      Updated!

      Will do the other areas in a different mr.

       
  • Rohan Verma

    Rohan Verma - 2016-07-26

    What do you think of opening PDFs in an iframe? Although I don't know if this will be compatible with all browsers.

     
    • Dave Brondsema

      Dave Brondsema - 2016-07-27

      I think PDFs can have security risks since they can have scripts embedded in them (but I'm not an expert). So we'd have to host them on a domain separate from the main domain. Which obviously would require extra setup and configuration, like having Allura running and responding to an additional domain somehow. Or possibly store attachments on disk ([#2560]) and then run a simple webserver for them, instead of Allura serving them.

       

      Related

      Tickets: #2560

      • Rohan Verma

        Rohan Verma - 2016-07-28

        I'm also not sure about the security aspect. Could you point me to some keywords so I can search more about this.

        Also wanted a clarification regarding [#2560] and [#6390], from what I understand you are suggesting that we should upload to a server and store the link to the file in the db instead of the file, right?

        Regarding the compatibility, I had found a MIT Licensed library that handles that https://pdfobject.com/ and also mozilla's https://mozilla.github.io/pdf.js/web/viewer.html (https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions). Would these be satisfying our requirements? (sample code: https://github.com/mozilla/pdf.js/tree/master/examples/helloworld)

         

        Related

        Tickets: #2560
        Tickets: #6390

        • Dave Brondsema

          Dave Brondsema - 2016-07-28

          I don't know much about the security aspect either :) The risk would be that you could embed javascript in a PDF (e.g. a script that reads document.cookies and sends it off somewhere for an attacker to hijack your session). If someone uploaded that PDF to an allura site, any visitor that loaded it could get their session hijacked. We had a similar vulnerabililty with SVG+JS a while ago [#8011] I googled around and can't find good information on what is possible, or even how to make a PDF w/ js in it so we could test (it is possible to put JS inside I know).

          For 2560/6390 yes, but I think that's a lot of extra work and probably not worth pursuing at this point, in my opinion.

          pdf.js is a good idea. It might not allow JS to run from within a PDF. But hard to say, since we don't know how PDF+JS works.

           

          Related

          Tickets: #8011

  • Dave Brondsema

    Dave Brondsema - 2016-07-27

    forgediscussion.tests.functional.test_forum:TestForumAsync.test_posts is failing. Otherwise looks good.

    I didn't know about the download attribute, that is nice. Makes [#6132] mostly taken care of (except for allowing text attachments to be displayed without download)

     

    Related

    Tickets: #6132

    • Rohan Verma

      Rohan Verma - 2016-07-30

      Updated the test. Yes I guess [#6132] is taken care of. I will look into the PDF problem it a bit further this weekend.

       

      Related

      Tickets: #6132

  • Dave Brondsema

    Dave Brondsema - 2016-08-01
    • Status: open --> merged
     
  • Dave Brondsema

    Dave Brondsema - 2016-08-01

    Added another commit to fix JS from running too much and making it difficult to close the lightboxes: [cd3b40]

     

Log in to post a comment.