Git Merge Request #255: [#8202] Personal Dashboard - Basics of Dashboard (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Deshani wants to merge 0 commits from /u/deshani/allura-personal-dashboard/ to master, 2018-06-11

Determining commits...

Discussion

  • Dave Brondsema

    Dave Brondsema - 2018-05-31

    Looking good! The basics are all here and seem good to me.

    However, the root redirects that were added are causing a number of tests to fail. We'll want those to be updated to pass before merging this. E.g. changing them to follow the redirects or access /neighborhood directly.

    Ideas for next steps (doesn't have to be done on this merge request):

    • add a basic functional test for /dashboard just to assert that "Tickets" appears in it or something real simple like that. You've got more tests planned for later steps, but it is good practice to have tests as soon as is practical. Keeping it simple is good, and helps avoid rewriting tests as the code/templates change.
    • right now there is a lot in common between UserProfileApp.profile_sections and DashboardController.index. And ProfileSectionBase and DashboardSectionBase. It might be good to extract common functionality into helper methods or an abstract parent class, so that code isn't duplicated. As the dashboard code grows, however, these methods might not be as similar in the long-term. So something to think about as you work on it all.
     

    • Deshani

      Deshani - 2018-05-31

      Thanks for the detailed explanation and sorry, I forgot to check the existing test cases. Will do the above changes as my next task

       

  • Dave Brondsema

    Dave Brondsema - 2018-06-04

    Another thing I noticed just now, if you go to Allura without a referer, you get an error when it tries to check request.referer

     

    • Deshani

      Deshani - 2018-06-04

      Thanks. I'll check it out.

       

  • Dave Brondsema

    Dave Brondsema - 2018-06-11
    • Status: open --> merged
     

Log in to post a comment.