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

Project E1936 doesn't save specialized rubrics #1475

Closed
efg opened this issue May 28, 2019 · 1 comment
Closed

Project E1936 doesn't save specialized rubrics #1475

efg opened this issue May 28, 2019 · 1 comment

Comments

@efg
Copy link
Member

efg commented May 28, 2019

We merged project E1936, Specialized Rubrics (#1444 ) but had trouble with it. This project makes it possible to specify specialized rubrics for each topic type, but not save these rubrics. That is, if you go to the Topics tab and select a specialized rubric, say Mozilla test (http://localhost:3000/questionnaires/view?id=647) for a particular topic, and hit the Save button at the bottom of the page, when the page redisplays, the dropdown for the topic will still say the rubric that was used before you tried to change the rubric to Mozilla test. That is to say, after changing rubrics for particular topics, there is no way to save them.

We poked around and found that (1) after an attempt to change a rubric, the pre-existing assignment_questionnaires entry was deleted rather than changed. A new entry had been created to take its place. Then we used a sql command to change the topic_id field of one of the assignment_questionnaires entries. Then we visited the topics tab again, all of the previous assignments_questionnaires entries were gone, and the new assignments_questionnaires entries all specified the rubric that is listed on the Rubrics tab.

mysql> select * from assignment_questionnaires where assignment_id = 982 and topic_id is not null;
+-------+---------------+------------------+---------+--------------------+----------------------+---------------+----------+----------+
| id    | assignment_id | questionnaire_id | user_id | notification_limit | questionnaire_weight | used_in_round | dropdown | topic_id |
+-------+---------------+------------------+---------+--------------------+----------------------+---------------+----------+----------+
| 47944 |           982 |              648 |    NULL |                 15 |                  100 |             1 |        1 |     4725 |
| 47945 |           982 |              647 |    NULL |                 15 |                  100 |             1 |        1 |     4726 |
| 47946 |           982 |              649 |    NULL |                 15 |                    0 |             2 |        1 |     4725 |
| 47947 |           982 |              649 |    NULL |                 15 |                    0 |             2 |        1 |     4726 |
| 47948 |           982 |               22 |    NULL |                 15 |                    0 |          NULL |        1 |     4725 |
| 47949 |           982 |               22 |    NULL |                 15 |                    0 |          NULL |        1 |     4726 |
+-------+---------------+------------------+---------+--------------------+----------------------+---------------+----------+----------+
6 rows in set (0.00 sec)

mysql> select * from assignment_questionnaires where assignment_id = 982 and topic_id is not null;
+-------+---------------+------------------+---------+--------------------+----------------------+---------------+----------+----------+
| id    | assignment_id | questionnaire_id | user_id | notification_limit | questionnaire_weight | used_in_round | dropdown | topic_id |
+-------+---------------+------------------+---------+--------------------+----------------------+---------------+----------+----------+
| 47950 |           982 |              648 |    NULL |                 15 |                  100 |             1 |        1 |     4725 |
| 47951 |           982 |              648 |    NULL |                 15 |                  100 |             1 |        1 |     4726 |
| 47952 |           982 |              649 |    NULL |                 15 |                    0 |             2 |        1 |     4725 |
| 47953 |           982 |              649 |    NULL |                 15 |                    0 |             2 |        1 |     4726 |
| 47954 |           982 |               22 |    NULL |                 15 |                    0 |          NULL |        1 |     4725 |
| 47955 |           982 |               22 |    NULL |                 15 |                    0 |          NULL |        1 |     4726 |
+-------+---------------+------------------+---------+--------------------+----------------------+---------------+----------+----------+
6 rows in set (0.00 sec)

So regardless of what changes are made on the Topics tab, the rubrics specified on the Rubrics tab are used for all topics.

nikolaygtitov added a commit to gabalmat/expertiza that referenced this issue Jun 10, 2019
  - Topics tab data gets overwritten by Rubrics tab due to design flaw
    that was discovered during debugging session
  - Required complete re-design of the project and re-implementation
  - New Design:
    * Two additional columns are added into the Assignment table that
      determines whether Rubrics varies by either Round or Topic with
      default values False
      - vary_by_round
      - vary by_topic
    * update_assignment_questionnaires method is re-implemented
      - Having extra column in the QA table topic_id, no need for
        deleting all the data and re-writing it again every single time
        in the DB
    * The only varying value is questionnaire_id, the rest values may
      not change from Topics or Rubrics tabs, but can be added
    * There are 4 (four) possible cases for saving and updating data:
      - used_in_round = null and topic_id = null
        * Indicates that update is received from Rubrics tab and Rubric
          does not vary by round. Update/enter the value for the type of
          questionnaire specified in the data. Example:
        id = 1 questionnaire_id = 1 used_in_round = null topic_id = null
      - used_in_round = integer and topic_id = null
        * Indicates that update is received from Rubrics tab and Rubric
          varies by specified rounds. Update/enter questionnaire_id for
          each round specified in the data. Example:
        id = 2 questionnaire_id = 1 used_in_round = 1 topic_id = null
        id = 3 questionnaire_id = 1 used_in_round = 2 topic_id = null
        id = 4 questionnaire_id = 1 used_in_round = 3 topic_id = null
      - used_in_round = null and topic_id = integer
        * Indicates that update is received from Topics tab, but Rubrics
          does not vary by round. Update/enter questionnaire_id for each
          corresponding task specified in the data. Example:
        id = 5 questionnaire_id = 1 used_in_round = null topic_id = 1
        id = 6 questionnaire_id = 1 used_in_round = null topic_id = 2
      - used_in_round = integer and topic_id = integer
        * Indicates that update id received from Topics tab and rubrics
          varies by round as well. Update/enter questionnaire_id for
          each corresponding task and round specified in data. Example:
        id = 7 questionnaire_id = 1 used_in_round = 1 topic_id = 1
        id = 8 questionnaire_id = 1 used_in_round = 2 topic_id = 1
        id = 9 questionnaire_id = 1 used_in_round = 1 topic_id = 2
        id = 10 questionnaire_id = 1 used_in_round = 2 topic_id = 2
  - Described design simplifies currently convoluted implementation and
    is a lot easier. It also helps to retrieve the data for Topics and
    Rubrics tabs.

TODO:
  - Fix all related Rspec Tests
  - All related Rspec Tests are Failing
nikolaygtitov added a commit to gabalmat/expertiza that referenced this issue Jun 13, 2019
  - Resolving this as part of the issue expertiza#1475
  - All spec/models are fixed and new Rspec tests are added to test
    newly implemented code based on the new design
    * Files that are completely re-worked:
      - assignment_form_spec.rb
        * context 'when attributes are not nil and received from Rubrics
          tab'
        * context 'when attributes are not nil and received from Topics
          tab'
      - assignment_spec.rb describe '#review_questionnaire_id'
      - review_response_map_spec.rb describe '#questionnaire'
      - self_review_response_map_spec.rb entire file
  - All duplicate methods that implement the same functionality but in
    different classes are re-worked to keep concrete logic implementation
    only in one single class
    * Duplicate methods in other classes all invoke this single logic
      implementation instead of each class has its own duplicate code
  - For the Topics tab if Questionnaire is not found, use
    "--Default rubric--" instead of "--None--"

TODO:
  - Fix remaining failing Rspec Tests
  - Test all the code changes manually
nikolaygtitov added a commit to gabalmat/expertiza that referenced this issue Jun 14, 2019
  - Resolving this as part of the issue expertiza#1475
  - All spec/helpers are fixed, no new Rspec tests are added:
    * Two tests are completely removed from assignment_helper_spec.rb:
      - AssignmentHelper#assignment_questionnaire"
      - AssignmentHelper#questionnaire"
    * Both methods are removed from the helpers/assignment_helper.rb
      methods contained duplicate implementation found in the different
      files models/assignment.rb (Assignment class),
      models/assignment_form.rb (AssignmentForm class), and others
    * To avoid all duplicate implementation, these methods are preserved
      only in the models/assignment_form.rb file:
      - AssignmentForm#assignment_questionnaire
      - AssignmentForm#questionnaire
    * All other duplicate implementations are revisited and removed

TODO:
  - Fix remaining failing Rspec Tests (spec/controllers)
  - Test all the code changes manually
nikolaygtitov added a commit to gabalmat/expertiza that referenced this issue Jun 17, 2019
  - Fixing failed Rspec tests as part of new design and implementation
    based on the issue expertiza#1475
  - This commit concludes the testing part for Rspec Controllers and
    Models
  - All changes are tested manually and working as expected:
    * Data in the Topics tab does not get overwritten by data from the
      Rubrics tab
    * All the data for Rubrics and Topics tabs is getting properly
      created and updated
    * There is no reason of deleting and re-writing DB multiple times
      with the same data on update() call:
      - Save data only once if data does not exist
      - If data already exists, then only update attributes is required
  - All Rspec tests are fixed to align with new design and
    implementation
  - Controllers changes:
    * Unnecessary comments are removed
    * All existing Rspec tests are modified to test new implementation
      and all pass successfully
    * No new respect tests are developed
  - Models changes:
    * Some models are reworked from the last commit to account for
      edge-case scenarios
    * Rspec tests are also reworked:
      - No need to create a real object and test them
      - Instead Mock all the necessary objects and allow to receive
        expected messages
      - New Rspec tests are added to test edge-case scenarios
      - Other Rspec tests from previous commit are removed due to
        redundancy and over-commit
      - Only necessary Rspec tests are remain in the files
      - All Rspec tests are modified and added pass successfully

TODO:
  - Fix spec/features failing Rspec Tests
  - There are failures in spec/features that are related to changes done
    previously in E1936
  - Flash was added each time the tab is switch and this may cause some
    failures in the spec/features
@Saurabh110
Copy link
Collaborator

Fixed by Pull Request #1444

@duhaoze11 duhaoze11 mentioned this issue Oct 4, 2021
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

No branches or pull requests

2 participants