Ticket #1932 (closed unplanned_task: obsolete)

Opened 10 years ago

Last modified 10 years ago

META_CHANGES_R0

Reported by: dido Owned by: mira
Priority: critical Milestone:
Component: BASE_RESOURCE_MODEL Version: 2.0
Keywords: Cc: mira
Category: BASE Effort:
Importance: Ticket_group:
Estimated Number of Hours: 0 Add Hours to Ticket: 0
Billable?: yes Total Hours: 0
Analysis_owners: mira Design_owners: mira
Imp._owners: mira Test_owners:
Analysis_reviewers: meddle Changelog:
Design_reviewers: meddle Imp._reviewers: meddle, pap
Test_reviewers: Analysis_score: 3.5
Design_score: 3 Imp._score: 3.5
Test_score: 0

Description

Change History

comment:1 Changed 10 years ago by mira

  • Owner set to mira
  • Status changed from new to s1a_analysis_started

comment:2 Changed 10 years ago by mira

  • Status changed from s1a_analysis_started to s1b_analysis_finished
  • Analysis_owners set to mira

comment:3 Changed 10 years ago by meddle

  • Status changed from s1b_analysis_finished to s1c_analysis_ok
  • Analysis_reviewers set to meddle
  • Analysis_score changed from 0 to 3.5

For a technical analysis, I think it passes...

3.5p (10m)

comment:4 Changed 10 years ago by mira

  • Design_owners set to mira
  • Status changed from s1c_analysis_ok to s2a_design_started

comment:5 Changed 10 years ago by mira

  • Status changed from s2a_design_started to s2b_design_finished

comment:6 Changed 10 years ago by meddle

  • Cc mira added
  • Design_score changed from 0 to 3
  • Design_reviewers set to meddle
  • Status changed from s2b_design_finished to s2c_design_ok
  • Notes about the test:
    • Your test tests the logic for meta changes that are defined in the org.sophie2.base.model.resources.r4 module. The problem is that it is not in it and in a module that depends on it. I understand that you wanted to use the resources from the book model, but you could write a small hierarchy of dummy ones for this test.
    • The way you wrote the testSkipFirsRevison method it tested nothing but the assertEquals method of the Unit tests, I fixed that...
    • The names of your methods, for testing are connected with the book model too testSkipMoveChange instead testSkipInsignificantChange for example.
  • As for the implementation up to now...
    • The revision's applyChange method is better to throw it's errors and every logic that applies a change to handle them differently, for now this implementation is working but real exceptions in the change logic will be hard to locate.
    • Similar problem is with MetaChange#skipRevision; run your test, you make assertations that are swallowed, your test for the first revision for example should catch the assertationerror thrown... I think that behavior is bad.
    • Your MetaChange needs factory methods like makeUndo(access), makeRedo(access), makeSkip(access, revison) or something like that, because writing meta changes with this big constructor is ugly...

So I think your design is good, but you were too lazy :) to write a book model independent test, so I'll pass it and you rewrite the test... As for the implementation, it will work like that, but you should talk with Milo and me what we will do with that throwable caching. For the implementation review write the factory methods of MetaChange.

3p (100m)

comment:7 Changed 10 years ago by meddle

Umm other thing next time put the right revisions :)

comment:8 Changed 10 years ago by mira

  • Status changed from s2c_design_ok to s3a_implementation_started

comment:9 Changed 10 years ago by mira

  • Imp._owners set to mira

comment:10 Changed 10 years ago by mira

  • Status changed from s3a_implementation_started to s3b_implementation_finished

I fixed the pointed ishues and there is also a user logic for making undo and redo

comment:11 Changed 10 years ago by pap

  • Status changed from s3b_implementation_finished to s3c_implementation_ok
  • Imp._score changed from 0 to 3.5
  • Imp._reviewers set to meddle, pap
  • In UndoRedoTest you could've used methods for creating pages and frames from ModelTest. This makes the test more readable.
  • You could've added a method for creating a main window in AppTestBase. This is the way test helpers become useful.
  • It is quite unnice to commit code with warnings.
  • Fake JavaDoc is something very bad.
  • Don't make meddle write the proper testing classes. Write the beforementioned yourself.
  • UndoRedoLogic's logic is incorrect. You should not assert that there is a current BookDocView. Maybe you can check whether there is such and if not just don't do anything.
  • Remember that there are other kinds of documents.
  • The above even renders the enumeration name incorrect.
  • Anyway we are very glad that we can perform such valuable actions at last.

comment:12 Changed 10 years ago by deyan

  • Status changed from s3c_implementation_ok to closed
  • Resolution set to obsolete

Batch update from file query-obsoleted.csv

Note: See TracTickets for help on using tickets.