bounswe / bounswe2017group5

Interestr - Build your own interest-based community
MIT License
6 stars 3 forks source link

Data Template Validation #186

Closed karacasoft closed 6 years ago

karacasoft commented 6 years ago

About

Don't know why, but decided to work on this stuff...

The frontend for the default template stuff is broken if it relies on the API side of the project. Because it was relying on putting null to the template field. I think we should not allow null template field. A group should be created with a 'default' template. I'll leave this for now and return to building android.

Changes

Visuals

image

karacasoft commented 6 years ago

Waiting for @ThoAppelsin 's review... :+1:

ThoAppelsin commented 6 years ago

I will do this, and many other work that I've missed, tomorrow. Computer Vision homework had taken a hold of me.

karacasoft commented 6 years ago

It's ok. This can stay here for a while, since it has very low priority

karacasoft commented 6 years ago

I tl;dr'd your paragraphs. Please, let me know if I missed a point

ThoAppelsin commented 6 years ago

It is very rude and uncooperative of you to not read the reviews I've made thoroughly. You do seem to have read all my points, though, so it is rather only just very rude of you.

karacasoft commented 6 years ago

@ThoAppelsin Let's do an interactive lesson about writing less stuff:

Checked it: We currently do pass an empty string as a response for when a multiple-select is submitted without any selections.

Should a response really be always non-empty?

Not that I am really fond of it, but currently we process the post inputs on the front-end and turn them into static strings of responses, hence we have just two fields, question and response, although the template schema is nowhere that so simple. We have our data structured there.

As a result of this, we , for example, concatenate the checked items on a multiple-select field, separated by commas, and store that as the response. How do we convert a multiple-select field when there are no options selected?

We already might be presenting may present something special such as (none), or maybe not. If we are not, then we maybe can. However, I think it is better to just allow an empty response, and display that as a "(none)" or whatever we wish on our front-ends.

Here I have read one of your long responses and converted into a much shorter passage that conveys absolutely the same meaning.

Let's do a part by part examination:

Checked it: We currently do pass an empty string as a response for when a multiple-select is submitted without any selections.

No problem so far. You are stating something I might not know. Can be worked on to be shortened, but it's ok.

Should a response really be always non-empty?

Good question.

Not that I am really fond of it, but currently we process the post inputs on the front-end and turn them into static strings of responses, hence we have just two fields, question and response, although the template schema is nowhere that so simple. We have our data structured there.

You are just writing what we already know. I mean if I'm working on validating this stuff, I think I should know how our data structure is. 3 lines long text (on desktop) that serves no purpose.

As a result of this, we, for example, concatenate the checked items on a multiple-select field, separated by commas, and store that as the response. How do we convert a multiple-select field when there are no options selected?

Good stuff, but mostly not related to this issue. We were talking about leaving response field empty. No need to waste other people's time with this. But we need to have connection with the next sentence. I would probably combine these two paragraphs into one, but for simplicity I'll just simplify them both.

We already might be presenting something special such as (none), or maybe not. If we are not, then we maybe can. However, I think it is better to just allow an empty response, and display that as a "(none)" or whatever we wish on our front-ends.

The only relevant part here is about allowing empty responses being better. So we just remove whatever that's not about it. But adding a special "(none)" text idea is cool and can be shared with others to be implemented later. So we leave that here, too.

The point is, I should be able to see my mistakes directly and do something about it as soon as I see your response. You don't have to prove something in your reviews. And I don't want to read an essay every single time I see your name in my mailbox.

AND, NO don't write a ~300 paragraphs answer for this. I will not read it. I'll simply return to my job now.

ThoAppelsin commented 6 years ago

You can never know how many lines it takes for the other side to fully comprehend what's going on in your mind in one-shot. I tend to take the safe route.

That long comment of mine which you took as an example got the following response from you:

Good point. I'll remove the check.

Easy-peasy. Took me only about 10-20 minutes to write that and get things done. Much better than starting off with 3 lines and then having a 3-hours-long discussion about something that we all should have learned about 3 years ago as we took the Cmpe 160. See: https://github.com/bounswe/bounswe2017group5/pull/186#discussion_r157476474

ThoAppelsin commented 6 years ago

This is just nonsense.

I will do the changes I recommended myself and seek for a review.

ThoAppelsin commented 6 years ago

I have just made a ~47 line commit considering the reviews I had made and re-opened this. This thing is valuable, and only needed a couple of fixes. It would be better if someone else reviewed it again, before we merge it.

karacasoft commented 6 years ago

Added one more fix in validate_fields. Please check