Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1283 survey edit update #301

Merged
merged 15 commits into from
Aug 12, 2016
Merged

1283 survey edit update #301

merged 15 commits into from
Aug 12, 2016

Conversation

willdoran
Copy link
Contributor

@willdoran willdoran commented Aug 3, 2016

This pull request makes the following changes:

  • API: Mark "Post" stage as special (maybe type: post vs type: task)
    • Post stage doesn't have complete/not completed
  • API: Add description and visibility for stages
  • Split post fields and task fields in survey settings
  • Split post fields and task fields in post edit
  • Split post fields and task field in post detail
  • Remove 'completed' toggle from "Post"

Test these changes by:

  • Survey Edit
  • Add Post fields
  • Add Tasks
  • Add fields to Task
  • Set "Require this task be completed before a post can be visible to the public"
  • Save survey
  • Post Edit/Detail
  • Create Post from Survey with No Tasks - tasks tabs should not be displayed
  • Create Post from Survey with Tasks - task tabs should be displayed
  • Save Post with title and description only - task tabs should not be displayed in Post Detail
  • Edit Post, add values to Task fields - task tabs and values should be displayed in Post Detail
  • Edit Post, with Task where "Require this task be completed before a post can be visible to the public", do not mark Task completed, attempt to save

Fixes ushahidi/platform#1283

Ping @ushahidi/platform


This change is Reviewable

@willdoran willdoran self-assigned this Aug 3, 2016
@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 3, 2016 23:20 Inactive
@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.03%) to 51.301% when pulling 616f602 on 1283-survey-edit-update into 539b43e on develop.

@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 4, 2016 16:30 Inactive
@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 8, 2016 17:29 Inactive
@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 8, 2016 18:01 Inactive
@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 9, 2016 13:26 Inactive
@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 9, 2016 13:35 Inactive
@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-1.5%) to 50.82% when pulling a566493 on 1283-survey-edit-update into f566eb4 on develop.

@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 9, 2016 14:19 Inactive
@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 9, 2016 14:33 Inactive
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 50.666% when pulling 6eacf3f on 1283-survey-edit-update into f566eb4 on develop.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-1.6%) to 50.666% when pulling 6eacf3f on 1283-survey-edit-update into f566eb4 on develop.

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-1.6%) to 50.666% when pulling bf997cf on 1283-survey-edit-update into f566eb4 on develop.

@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 9, 2016 16:03 Inactive
@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-1.6%) to 50.678% when pulling 099faab on 1283-survey-edit-update into f566eb4 on develop.

@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 9, 2016 21:10 Inactive
@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-1.6%) to 50.678% when pulling 129f3d2 on 1283-survey-edit-update into f566eb4 on develop.

@rjmackay
Copy link
Contributor

I see what you mean about the complexity in post detail.. we should probably try to include a refactor in a future sprint.


Reviewed 1 of 7 files at r1, 1 of 9 files at r2, 1 of 2 files at r3, 3 of 7 files at r4.
Review status: 6 of 14 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.


app/post/detail/post-detail.controller.js, line 116 [r5] (raw file):

            }, $scope.form_attributes);

            // Make the first task visible

I'd prefer that we popped the post stage out of the tasks array ie. so tasks contains only tasks an post_task contains the post task.
Otherwise this is fragile and may break depending on ordering.


app/post/detail/post-detail.controller.js, line 125 [r5] (raw file):

                //Set post task id
                if (task.type === 'post') {

Can we add a note that this is assuming post has only a single 'post' stage ??


app/post/detail/post-value.directive.js, line 13 [r5] (raw file):

        templateUrl: 'templates/posts/post-detail-value.html',
        link: function ($scope) {
            $scope.standardTask = $scope.type === 'standard';

... what is this for? Whats a standard task?


server/www/templates/settings/surveys/modify/editor.html, line 264 [r5] (raw file):

                  <div class="form-field switch">
                     <label translate="survey.require_task">Require this task be completed before a post can be visible to the public</label>

This probably need a for attribute.


server/www/templates/settings/surveys/modify/editor.html, line 266 [r5] (raw file):

                     <label translate="survey.require_task">Require this task be completed before a post can be visible to the public</label>
                     <div class="toggle-switch">
                        <input class="tgl" id="switch1" type="checkbox" ng-model="task.required">

Please give this is sensible id


server/www/templates/settings/surveys/modify/editor.html, line 267 [r5] (raw file):

                     <div class="toggle-switch">
                        <input class="tgl" id="switch1" type="checkbox" ng-model="task.required">
                        <label class="tgl-btn" for="switch1" ng-click="toggleTaskRequired(task)"></label>

Why does this need ng-click shouldn't clicking the label toggle the value of a checkbox anyway?


server/www/templates/settings/surveys/modify/editor.html, line 274 [r5] (raw file):

                      <label translate="survey.show_task">Show this task to everyone when the post is published</label>
                      <div class="toggle-switch">
                          <input class="tgl" id="custom-show" type="checkbox" ng-model="task.is_public">

needs a sane id


server/www/templates/settings/surveys/modify/survey-task-create.html, line 7 [r5] (raw file):

  </div>
  <div class="form-field">
      <label translate="survey.task_description">

needs a for attribute


server/www/templates/settings/surveys/modify/survey-task-create.html, line 15 [r5] (raw file):

     <label translate="survey.require_task">Require this task be completed before a post can be visible to the public</label>
     <div class="toggle-switch">
        <input class="tgl" id="switchnewtask" type="checkbox" ng-model="newTask.required">

Can we give this a better id like required-toggle?


server/www/templates/settings/surveys/modify/survey-task-create.html, line 20 [r5] (raw file):

  </div>
  <div class="form-field switch">
      <label translate="survey.show_task">Show this task to everyone when the post is published</label>

needs a for attribute


server/www/templates/settings/surveys/modify/survey-task-create.html, line 22 [r5] (raw file):

      <label translate="survey.show_task">Show this task to everyone when the post is published</label>
      <div class="toggle-switch">
          <input class="tgl" id="custom-show" type="checkbox" ng-model="newTask.is_public">

Needs a sane id value.


Comments from Reviewable

@willdoran
Copy link
Contributor Author

Review status: 6 of 14 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.


app/post/detail/post-detail.controller.js, line 116 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

I'd prefer that we popped the post stage out of the tasks array ie. so tasks contains only tasks an post_task contains the post task.
Otherwise this is fragile and may break depending on ordering.

Yep, I'll change that

app/post/detail/post-detail.controller.js, line 125 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

Can we add a note that this is assuming post has only a single 'post' stage ??

Done.

app/post/detail/post-value.directive.js, line 13 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

... what is this for? Whats a standard task?

Unfortunately the CSS changes depending on the section, so this swaps the classes, I'll add a comment

server/www/templates/settings/surveys/modify/editor.html, line 264 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

This probably need a for attribute.

Done.

server/www/templates/settings/surveys/modify/editor.html, line 266 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

Please give this is sensible id

Done.

server/www/templates/settings/surveys/modify/editor.html, line 274 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

needs a sane id

Done.

server/www/templates/settings/surveys/modify/survey-task-create.html, line 7 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

needs a for attribute

Done.

server/www/templates/settings/surveys/modify/survey-task-create.html, line 15 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

Can we give this a better id like required-toggle?

Done.

server/www/templates/settings/surveys/modify/survey-task-create.html, line 20 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

needs a for attribute

Done.

server/www/templates/settings/surveys/modify/survey-task-create.html, line 22 [r5] (raw file):

Previously, rjmackay (Robbie Mackay) wrote…

Needs a sane id value.

Done.

Comments from Reviewable

@rjmackay rjmackay temporarily deployed to ushahidi-platform-maste-pr-301 August 12, 2016 13:40 Inactive
@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage decreased (-1.6%) to 50.678% when pulling 94bbb87 on 1283-survey-edit-update into f566eb4 on develop.

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