physionet-build icon indicating copy to clipboard operation
physionet-build copied to clipboard

Make Create Project Flow descriptions configurable

Open matkaczmarek opened this issue 1 year ago • 9 comments

T-CAIREM 1137

We should make copies on create project flow easy to manage and change: image

matkaczmarek avatar Feb 23 '24 13:02 matkaczmarek

Thanks @matkaczmarek please could you include a full description for the pull request here? I don't think we have access to your issue tracker (or at least if we do, not all of us are using it!).

tompollard avatar Feb 23 '24 14:02 tompollard

Thanks @matkaczmarek please could you include a full description for the pull request here? I don't think we have access to your issue tracker (or at least if we do, not all of us are using it!).

Hi @tompollard I updated the description but keep in mind that this is just a proof of concept and still in WIP state. We will discuss the general approach to this feature with @Rutvikrj26 on Tuesday but I created this draft to demonstrate one of the potential approaches.

matkaczmarek avatar Feb 23 '24 14:02 matkaczmarek

Two questions for @tompollard, @bemoody:

  • should we implement defaults in views when step details is not available in database such as here(if/else): https://github.com/MIT-LCP/physionet-build/pull/2198/files#diff-4f961161fbbb6291c7505b39816715a53c104e67e49f1d9e90e5e78f5854ea23R40
  • class Section and class StepDetails are very similar, I introduced AbstractSection class but maybe it is overengineering: https://github.com/MIT-LCP/physionet-build/pull/2198/files#diff-5f08ef366490f7ccf136e57d79e24e97eaca87f08b8a86e89edd678d6324b763R62

Also I'm not very fond of naming those new class as Step/StepDetails, maybe you have better idea?

matkaczmarek avatar Mar 29 '24 11:03 matkaczmarek

(I haven't looked at the code at all, just responding to things we talked about in the meeting.)

should we implement defaults in views when step details is not available in database such as here

Not a great idea for a couple of reasons:

  • We don't want logic in the template files (as much as we can possibly avoid it.)

  • It's better not to have two different data flows to begin with (in this case, the "default" text being stored in a secondary template file, versus the "custom" text being stored in the database.)

A better approach is to move the default text into the database (using a data migration.)

class Section and class StepDetails are very similar, I introduced AbstractSection class but maybe it is overengineering

Yeah, these don't really belong together and trying to use an abstract class makes things needlessly complicated in my opinion.

Also I'm not very fond of naming those new class as Step/StepDetails, maybe you have better idea?

Words that come to mind: "form description", "help text", "boilerplate"?

I think that we would probably like to reuse the same model for things across the site that are not necessarily tied to the project-submission-workflow pages (for example, the user settings pages.)

bemoody avatar Apr 02 '24 14:04 bemoody

I've completed the implementation started by @matkaczmarek with most of the feedback incorporated by @bemoody. However, I could not come up with a better name and have kept it the same. [Further work on fixing tests pending]

@bemoody I do have the fixtures, but need some help with incorporating the fixtures (project_copy.json) in the physionet/fixtures into the migrations at the start of the deployment. Please let me know how would you like that to be implemented for physionet.

Rutvikrj26 avatar May 08 '24 21:05 Rutvikrj26

@bemoody I have re-generated the migrations as you suggested and it has resolved the issue. The PR is now open for review.

Further, this would require data seeding to put into the database at the start of the deployment right after table migrations. Are there any ways that you suggest we implement for that?

Rutvikrj26 avatar May 16 '24 07:05 Rutvikrj26

I keep coming back to thinking about translation, because the problem you're trying to solve here is very nearly the same problem that GNU gettext, and other i18n libraries, are meant to solve. If you haven't ever used gettext, you should read about it and understand how it works and why.

One of the distinguishing features of the GNU approach is that the message-ID is the same as the "untranslated" string (i.e., English is treated as the default.) This is a bit controversial; a competing philosophy says that message-ID should be a fixed identifier of some kind, and English should be treated the same as any other language.

But either approach recognizes that the messages are separate from the application logic, and they may evolve separately.

Suppose, for the sake of argument, that we wanted to move the Ethics Statement into the Content page, while moving the Supporting Documents to a new page after the Files page.

Then, suddenly, the former description for the Ethics page might or might not make any sense; not to mention, the order of "steps" wouldn't match what's in the database. As developers implementing this change, we would need to define a new set of defaults, while also trying to preserve existing customizations. That's, in essence, the problem that tools like xgettext/msgmerge are designed to solve.

I don't have a completely fleshed out idea here, but hopefully this is some food for thought.

bemoody avatar May 23 '24 22:05 bemoody

Summarizing the discussion during the standup, following two key points were mentioned:

  • This is a generic approach to scraping off any text and store it into database. The current implementation is infact a version a version of it, but a bit more structured. A generic message_id and a message_str for the entire site will be super confusing to both manage and implement.
  • Django has it's own gettext functionality for translation, which follows the structure of GNU gettext. We will need to write a custom utility function similar to the base command provided that rather than generating .mo files, generates fixtures and then these fixtures can be added to the database. This once again is a super intensive task, requiring a lot of effort, which is not worth the result.

@bemoody please add any points I might have missed during the discussion.

Rutvikrj26 avatar May 28 '24 19:05 Rutvikrj26

@bemoody @tompollard This is becoming a critical feature as more and more people uploading databases are getting confused. We would like to have this one merged as soon as possible. Is it possible to merge this one and refactor later once the feature is out there?

Rutvikrj26 avatar Aug 02 '24 17:08 Rutvikrj26