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

feat: review and form model ADR #2003

Open
wants to merge 6 commits into
base: master
from
Open

feat: review and form model ADR #2003

wants to merge 6 commits into from

Conversation

@Macroz
Copy link
Collaborator

Macroz commented Feb 13, 2020

No description provided.

Macroz and others added 5 commits Feb 13, 2020
@Macroz Macroz mentioned this pull request Feb 13, 2020
@Macroz Macroz mentioned this pull request Feb 24, 2020
2 of 21 tasks complete
Application contains form fields from all forms of all catalogue items.
Review request is sent with certain catalogue items selected and it controls which fields are shown to the reviewer.
4. Model each reviewer organization as a catalogue item with same form.
Model private fields as repeating fields that repeat per catalogue item.

This comment has been minimized.

Copy link
@opqdonut

opqdonut Feb 24, 2020

Collaborator

I don't understand option 4. Could you rephrase it?

This comment has been minimized.

Copy link
@Macroz

Macroz Feb 24, 2020

Author Collaborator

I have slightly adjusted the wording but we can try to fix it after f2f discussion.

Application contains form fields of one form.
Review request is sent with certain catalogue items selected and it controls which fields are shown to the reviewer.
5. Add a field type to the model where you can add multiple answers.
Model private fields as repeating fields that repeat per each answer to the multiple answer field.

This comment has been minimized.

Copy link
@opqdonut

opqdonut Feb 24, 2020

Collaborator

I think I understand option 5 but I think the description could be clearer

This comment has been minimized.

Copy link
@Macroz

Macroz Feb 24, 2020

Author Collaborator

Same as above.


In the **option 4** we would make a new repeating functionality that would repeat per applied catalogue item. Modeling each reviewer organization as a catalogue item seems like a business decision, not something for us. It requires the organization to either be able to statically know each option, or have a process for adding missing ones. While the repeating logic is sound, we can also consider **option 5** as the same, but not having the need to couple this into catalogue items. So lets prefer **option 5** and not this one.

A field type where you can "press add button" to add more rows sounds also like a natural extension to the types of fields we have. Also repeating fields based on the count of a previous answer sounds sensible. Some kind of grouping logic may be needed to repeat fields in a group (ABCABCABC and not AAABBBCCC). Therefore **option 5** seems like a sensible extension that we should consider.

This comment has been minimized.

Copy link
@opqdonut

opqdonut Feb 24, 2020

Collaborator

I'm not sure what these repeated fields are. Perhaps they should be covered in Background?

This comment has been minimized.

Copy link
@Macroz

Macroz Feb 24, 2020

Author Collaborator

They don't exist so they are not background. It is a concept that answer is repeated for some reason, not just one per field.


The tasks should be
1. organization creation
- store id, name, description, owners and review-email

This comment has been minimized.

Copy link
@opqdonut

opqdonut Feb 24, 2020

Collaborator

I'm not sure this concept for organizations is compatible with the current concept for organizations. First of all, currently we assume users are mapped to organizations by the idp, but in this case the mapping would be dynamic. Perhaps there wouldn't even be a need to map users to organizations since the review request constains the organization?

Could there be two different concepts for organization? This new one could be called a "visibility group" or something.

This comment has been minimized.

Copy link
@Macroz

Macroz Feb 24, 2020

Author Collaborator

Here are some considerations:

  • A review request can contain an organization but it may not contain a user. It is unknown by the handler who the reviewer will be.
  • Not all users logging into an organization should be able to see all reviews.
  • Does IDP organization for a reviewer actually match expected organization? I think the mapping to a specific application must be done like the invite member.
  • I don't see any problem in having one concept for an organization here and using it for two things. For example we can avoid having two separate but almost identical UI functions.
  • The organization that does reviews also likely wants to maintain their own forms and/or catalogue, which is exactly the organization model we have now?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.